From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([140.186.70.92]:37538)
	by lists.gnu.org with esmtp (Exim 4.71) (envelope-from <hch@lst.de>)
	id 1QOSxm-0005q9-71
	for qemu-devel@nongnu.org; Mon, 23 May 2011 07:06:19 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <hch@lst.de>) id 1QOSxl-0003KH-32
	for qemu-devel@nongnu.org; Mon, 23 May 2011 07:06:18 -0400
Received: from verein.lst.de ([213.95.11.211]:55384 helo=newverein.lst.de)
	by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from <hch@lst.de>)
	id 1QOSxk-0003KC-St
	for qemu-devel@nongnu.org; Mon, 23 May 2011 07:06:17 -0400
Date: Mon, 23 May 2011 13:06:15 +0200
From: Christoph Hellwig <hch@lst.de>
Message-ID: <20110523110615.GA15957@lst.de>
References: <4DDA3153.4070003@amd.com> <4DDA383F.7080908@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4DDA383F.7080908@amd.com>
Subject: Re: [Qemu-devel] [PATCH] block/raw-posix: get right size
	of	partition size
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Christoph Egger <Christoph.Egger@amd.com>
Cc: "qemu-devel@nongnu.org" <qemu-devel@nongnu.org>

On Mon, May 23, 2011 at 12:34:39PM +0200, Christoph Egger wrote:
>
> This does 2 things:
> - use the correct way to get the size of a disk device or partition
>     (from haad@NetBSD.org)
> - if given a block device, use the character device instead.
>     (from bouyer@NetBSD.org)

Please split that into two independent patches.

> +    if (S_ISLNK(sb.st_mode)) {
> +        fprintf(stderr, "%s: symbolic link not supported\n", filename);
> +        return -EINVAL;
> +    }

Why not, it's a pretty clear regression from current code, and will
break various Linux setups where there are lots of symlinks under /dev,
as well as users using symlinks for their image files.

> +#if defined(__NetBSD__)
> +    if (S_ISBLK(sb.st_mode)) {
> +        static char namebuf[PATH_MAX];
> +        const char *dp = strrchr(filename, '/');
> +
> +        if (dp == NULL) {
> +            snprintf(namebuf, PATH_MAX, "r%s", filename);
> +        } else {
> +            snprintf(namebuf, PATH_MAX, "%.*s/r%s",
> +                (int)(dp - filename), filename, dp + 1);
> +        }
> +        fprintf(stderr, "%s is a block device", filename);
> +        filename = namebuf;
> +        fprintf(stderr, ", using %s\n", filename);
> +    }
> +#endif

Please split this out into a helper, which has a no-op version
for other operating systems.  It probably should also be enabled
for other operating systems having char and block nodes for disk
devices.  That's at least OpenBSD and Solaris, not sure about
Darwin.

> -#ifdef __OpenBSD__
> +#if defined(__OpenBSD__) || defined(__NetBSD__)
>  static int64_t raw_getlength(BlockDriverState *bs)
>  {
>      BDRVRawState *s = bs->opaque;
> @@ -613,12 +645,22 @@ static int64_t raw_getlength(BlockDriverState *bs)
>      if (fstat(fd, &st))
>          return -1;
>      if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
> -        struct disklabel dl;
> +#if defined(__NetBSD__)
> +        struct dkwedge_info dkw;
>
> -        if (ioctl(fd, DIOCGDINFO, &dl))
> -            return -1;
> -        return (uint64_t)dl.d_secsize *
> -            dl.d_partitions[DISKPART(st.st_rdev)].p_size;
> +        if (ioctl(fd, DIOCGWEDGEINFO, &dkw) != -1) {
> +            return dkw.dkw_size * 512;
> +        } else {
> +#endif

Please provide a completely separate raw_getlength for NetBSD instead
of creating this ifdef mess for almost no shared code.