| Message ID | 4DDA5442.30801@amd.com |
|---|---|
| State | New |
| Headers | show |
> + if (lstat(filename, &sb) < 0) { > + fprintf(stderr, "%s: stat failed: %s\n", filename, > strerror(errno)); > + return -errno; > + } > + > + if (S_ISBLK(sb.st_mode)) > + filename = raw_get_rawdevice(filename); Please move the lstat and S_ISBLK check into raw_get_rawdevice. Also it might be worth to rename it to something like raw_normalize_devicepath.
On 05/23/11 15:42, Christoph Hellwig wrote: >> + if (lstat(filename,&sb)< 0) { >> + fprintf(stderr, "%s: stat failed: %s\n", filename, >> strerror(errno)); >> + return -errno; >> + } >> + >> + if (S_ISBLK(sb.st_mode)) >> + filename = raw_get_rawdevice(filename); > > Please move the lstat and S_ISBLK check into raw_get_rawdevice. Why? What raw_get_rawdevice() does is only relevant to block devices and this hunk makes that clear. Christoph > Also it might be worth to rename it to something like raw_normalize_devicepath.
Am 23.05.2011 14:34, schrieb Christoph Egger: > > if given a block device, use the character device instead. > > From: Manuel Bouyer <bouyer@NetBSD.org> > Signed-off-by: Christoph Egger <Christoph.Egger@amd.com> A useful commit message would explain why you're doing that. > > diff --git a/block/raw-posix.c b/block/raw-posix.c > index 6b72470..d05f373 100644 > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > @@ -136,11 +143,45 @@ static int64_t raw_getlength(BlockDriverState *bs); > static int cdrom_reopen(BlockDriverState *bs); > #endif > > +#if defined(__NetBSD__) > +static const char *raw_get_rawdevice(const char *filename) > +{ > + 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); Not sure if fprintf is a good idea here, but ok. > + > + return filename; > +} > +#else > +static const char *raw_get_rawdevice(const char *filename) > +{ > + return filename; > +} > +#endif > + > static int raw_open_common(BlockDriverState *bs, const char *filename, > int bdrv_flags, int open_flags) > { > 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)); The patch is corrupted by this line wrap. Please remove the fprintf, the callers are responsible for sending an error message to the right destination. > + return -errno; > + } > + > + if (S_ISBLK(sb.st_mode)) > + filename = raw_get_rawdevice(filename); The qemu coding style requires braces. Also, I agree with Christoph that the lstat/S_ISBLK should be moved into the NetBSD specific code. Kevin
On 05/23/11 16:11, Kevin Wolf wrote: > Am 23.05.2011 14:34, schrieb Christoph Egger: >> >> if given a block device, use the character device instead. >> >> From: Manuel Bouyer<bouyer@NetBSD.org> >> Signed-off-by: Christoph Egger<Christoph.Egger@amd.com> > > A useful commit message would explain why you're doing that. How about this: On NetBSD, the PV backend has to use the block device; but a userland process is better with the character device interface. In addition, a block device can't be opened twice; if the backend opens it qemu can't and vice-versa. >> >> diff --git a/block/raw-posix.c b/block/raw-posix.c >> index 6b72470..d05f373 100644 >> --- a/block/raw-posix.c >> +++ b/block/raw-posix.c >> @@ -136,11 +143,45 @@ static int64_t raw_getlength(BlockDriverState *bs); >> static int cdrom_reopen(BlockDriverState *bs); >> #endif >> >> +#if defined(__NetBSD__) >> +static const char *raw_get_rawdevice(const char *filename) >> +{ >> + 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); > > Not sure if fprintf is a good idea here, but ok. I want to make it clear what file the qemu process has been using. this is what log files are for, isn't it ? >> + >> + return filename; >> +} >> +#else >> +static const char *raw_get_rawdevice(const char *filename) >> +{ >> + return filename; >> +} >> +#endif >> + >> static int raw_open_common(BlockDriverState *bs, const char *filename, >> int bdrv_flags, int open_flags) >> { >> 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)); > > The patch is corrupted by this line wrap. this line fits into 80 columns. must be a mail client or a ms exchange problem. > > Please remove the fprintf, the callers are responsible for sending an > error message to the right destination. >> + return -errno; >> + } >> + >> + if (S_ISBLK(sb.st_mode)) >> + filename = raw_get_rawdevice(filename); > > The qemu coding style requires braces. > > Also, I agree with Christoph that the lstat/S_ISBLK should be moved into > the NetBSD specific code. Ok. Christoph
Am 24.05.2011 10:36, schrieb Christoph Egger: > On 05/23/11 16:11, Kevin Wolf wrote: >> Am 23.05.2011 14:34, schrieb Christoph Egger: >>> >>> if given a block device, use the character device instead. >>> >>> From: Manuel Bouyer<bouyer@NetBSD.org> >>> Signed-off-by: Christoph Egger<Christoph.Egger@amd.com> >> >> A useful commit message would explain why you're doing that. > > How about this: > > On NetBSD, the PV backend has to use the block device; but a > userland process is better with the character device interface. In > addition, a block device can't be opened twice; if the backend opens > it qemu can't and vice-versa. Hm, what PV backend? Are you talking about Xen? If so, let's make this clear: On NetBSD a userland process is better with the character device interface. In addition, a block device can't be opened twice; if a Xen backend opens it, qemu can't and vice-versa. >>> diff --git a/block/raw-posix.c b/block/raw-posix.c >>> index 6b72470..d05f373 100644 >>> --- a/block/raw-posix.c >>> +++ b/block/raw-posix.c >>> @@ -136,11 +143,45 @@ static int64_t raw_getlength(BlockDriverState *bs); >>> static int cdrom_reopen(BlockDriverState *bs); >>> #endif >>> >>> +#if defined(__NetBSD__) >>> +static const char *raw_get_rawdevice(const char *filename) >>> +{ >>> + 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); >> >> Not sure if fprintf is a good idea here, but ok. > > I want to make it clear what file the qemu process has been > using. this is what log files are for, isn't it ? Yeah, while I don't like fprintfs in block driver code, I do agree that it makes some sense here. Kevin
On 05/24/11 11:10, Kevin Wolf wrote: > Am 24.05.2011 10:36, schrieb Christoph Egger: >> On 05/23/11 16:11, Kevin Wolf wrote: >>> Am 23.05.2011 14:34, schrieb Christoph Egger: >>>> >>>> if given a block device, use the character device instead. >>>> >>>> From: Manuel Bouyer<bouyer@NetBSD.org> >>>> Signed-off-by: Christoph Egger<Christoph.Egger@amd.com> >>> >>> A useful commit message would explain why you're doing that. >> >> How about this: >> >> On NetBSD, the PV backend has to use the block device; but a >> userland process is better with the character device interface. In >> addition, a block device can't be opened twice; if the backend opens >> it qemu can't and vice-versa. > > Hm, what PV backend? Are you talking about Xen? If so, let's make this > clear: Yes, it is about Xen. > On NetBSD a userland process is better with the character device > interface. In addition, a block device can't be opened twice; if a Xen > backend opens it, qemu can't and vice-versa. > >>>> diff --git a/block/raw-posix.c b/block/raw-posix.c >>>> index 6b72470..d05f373 100644 >>>> --- a/block/raw-posix.c >>>> +++ b/block/raw-posix.c >>>> @@ -136,11 +143,45 @@ static int64_t raw_getlength(BlockDriverState *bs); >>>> static int cdrom_reopen(BlockDriverState *bs); >>>> #endif >>>> >>>> +#if defined(__NetBSD__) >>>> +static const char *raw_get_rawdevice(const char *filename) >>>> +{ >>>> + 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); >>> >>> Not sure if fprintf is a good idea here, but ok. >> >> I want to make it clear what file the qemu process has been >> using. this is what log files are for, isn't it ? > > Yeah, while I don't like fprintfs in block driver code, I do agree that > it makes some sense here. > > Kevin >
diff --git a/block/raw-posix.c b/block/raw-posix.c index 6b72470..d05f373 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -136,11 +143,45 @@ static int64_t raw_getlength(BlockDriverState *bs); static int cdrom_reopen(BlockDriverState *bs); #endif +#if defined(__NetBSD__) +static const char *raw_get_rawdevice(const char *filename) +{ + 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); + + return filename; +} +#else +static const char *raw_get_rawdevice(const char *filename) +{ + return filename; +} +#endif + static int raw_open_common(BlockDriverState *bs, const char *filename, int bdrv_flags, int open_flags) { 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_ISBLK(sb.st_mode)) + filename = raw_get_rawdevice(filename); s->open_flags = open_flags | O_BINARY;