| Message ID | 4DDA383F.7080908@amd.com |
|---|---|
| State | New |
| Headers | show |
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.
On 05/23/11 13:06, Christoph Hellwig wrote: > 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. ok. > >> + 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. there's no easy way to find the real target of a symlink, to find the corresponding character device (this could be refinded, because this restriction only applies if the target is a block device). > >> +#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. Yes, but don't have a test machine right now. >> -#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. Ok.
diff --git a/block/raw-posix.c b/block/raw-posix.c index 6b72470..7cf6461 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -64,6 +64,13 @@ #include <sys/dkio.h> #endif +#ifdef __NetBSD__ +#include <sys/ioctl.h> +#include <sys/disklabel.h> +#include <sys/dkio.h> +#include <sys/disk.h> +#endif + #ifdef __DragonFly__ #include <sys/ioctl.h> #include <sys/diskslice.h> @@ -141,7 +148,32 @@ static int raw_open_common(BlockDriverState *bs, const char *filename, { BDRVRawState *s = bs->opaque; int fd, ret; + struct stat sb; + if (lstat(filename, &sb) < 0) { + fprintf(stderr, "%s: stat failed: %s\n", filename, strerror(errno)); + return -errno; + } + if (S_ISLNK(sb.st_mode)) { + fprintf(stderr, "%s: symbolic link not supported\n", filename); + return -EINVAL; + } +#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 s->open_flags = open_flags | O_BINARY;
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) From: Adam Hamsik <haad@netbsd.org> From: Manuel Bouyer <bouyer@netbsd.org> Signed-off-by: Christoph Egger <Christoph.Egger@amd.com> s->open_flags &= ~O_ACCMODE; if (bdrv_flags & BDRV_O_RDWR) { @@ -603,7 +635,7 @@ static int raw_truncate(BlockDriverState *bs, int64_t offset) return 0; } -#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 + struct disklabel dl; + + if (ioctl(fd, DIOCGDINFO, &dl)) + return -1; + return (uint64_t)dl.d_secsize * + dl.d_partitions[DISKPART(st.st_rdev)].p_size; +#if defined(__NetBSD__) + } +#endif } else return st.st_size; }