[Qemu-devel] [PATCH] block/raw-posix: use a character device if a block device is given

[Qemu-devel] [PATCH] block/raw-posix: use a character device if a block device is given

[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>

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;
      s->open_flags &= ~O_ACCMODE;


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [Qemu-devel] [PATCH] block/raw-posix: use a character device if a block device is given

[Christoph Hellwig]

> +    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.

Re: [Qemu-devel] [PATCH] block/raw-posix: use a character device if a block device is given

[Christoph Egger]

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.


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [Qemu-devel] [PATCH] block/raw-posix: use a character device if a block device is given

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH] block/raw-posix: use a character device if a block device is given

[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.

>>
>> 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


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [Qemu-devel] [PATCH] block/raw-posix: use a character device if a block device is given

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH] block/raw-posix: use a character device if a block device is given

[Christoph Egger]

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
>


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [Qemu-devel] [PATCH] block/raw-posix: use a character device if a block device is given

[Kevin Wolf]

Am 25.05.2011 14:19, schrieb Christoph Egger:
> On 05/25/11 12:43, Kevin Wolf wrote:
>> Am 24.05.2011 11:30, schrieb Christoph Egger:
>>>
>>> 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.
>>
>> If you provide a Signed-off-by, I think this patch is ready to be committed.
> 
> Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [PATCH] block/raw-posix: use a character device if a block device is given

[Christoph Egger]

On 05/25/11 12:43, Kevin Wolf wrote:
> Am 24.05.2011 11:30, schrieb Christoph Egger:
>>
>> 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.
>
> If you provide a Signed-off-by, I think this patch is ready to be committed.

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>

> Kevin
>
>>
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index 6b72470..64dceb1 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -136,12 +143,55 @@ static int64_t raw_getlength(BlockDriverState *bs);
>>    static int cdrom_reopen(BlockDriverState *bs);
>>    #endif
>>
>> +#if defined(__NetBSD__)
>> +static int raw_normalize_devicepath(const char **filename)
>> +{
>> +    static char namebuf[PATH_MAX];
>> +    const char *dp, *fname;
>> +    struct stat sb;
>> +
>> +    fname = *filename;
>> +    dp = strrchr(fname, '/');
>> +    if (lstat(fname,&sb)<  0) {
>> +        fprintf(stderr, "%s: stat failed: %s\n",
>> +            fname, strerror(errno));
>> +        return -errno;
>> +    }
>> +
>> +    if (!S_ISBLK(sb.st_mode)) {
>> +        return 0;
>> +    }
>> +
>> +    if (dp == NULL) {
>> +        snprintf(namebuf, PATH_MAX, "r%s", fname);
>> +    } else {
>> +        snprintf(namebuf, PATH_MAX, "%.*s/r%s",
>> +            (int)(dp - fname), fname, dp + 1);
>> +    }
>> +    fprintf(stderr, "%s is a block device", fname);
>> +    *filename = namebuf;
>> +    fprintf(stderr, ", using %s\n", *filename);
>> +
>> +    return 0;
>> +}
>> +#else
>> +static int raw_normalize_devicepath(const char **filename)
>> +{
>> +    return 0;
>> +}
>> +#endif
>> +
>>    static int raw_open_common(BlockDriverState *bs, const char *filename,
>>                               int bdrv_flags, int open_flags)
>>    {
>>        BDRVRawState *s = bs->opaque;
>>        int fd, ret;
>>
>> +    ret = raw_normalize_devicepath(&filename);
>> +    if (ret != 0) {
>> +        return ret;
>> +    }
>> +
>>        s->open_flags = open_flags | O_BINARY;
>>        s->open_flags&= ~O_ACCMODE;
>>        if (bdrv_flags&  BDRV_O_RDWR) {
>>
>>
>
>


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [Qemu-devel] [PATCH] block/raw-posix: use a character device if a block device is given

[Kevin Wolf]

Am 24.05.2011 11:30, schrieb Christoph Egger:
> 
> 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.

If you provide a Signed-off-by, I think this patch is ready to be committed.

Kevin

> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 6b72470..64dceb1 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -136,12 +143,55 @@ static int64_t raw_getlength(BlockDriverState *bs);
>   static int cdrom_reopen(BlockDriverState *bs);
>   #endif
> 
> +#if defined(__NetBSD__)
> +static int raw_normalize_devicepath(const char **filename)
> +{
> +    static char namebuf[PATH_MAX];
> +    const char *dp, *fname;
> +    struct stat sb;
> +
> +    fname = *filename;
> +    dp = strrchr(fname, '/');
> +    if (lstat(fname, &sb) < 0) {
> +        fprintf(stderr, "%s: stat failed: %s\n",
> +            fname, strerror(errno));
> +        return -errno;
> +    }
> +
> +    if (!S_ISBLK(sb.st_mode)) {
> +        return 0;
> +    }
> +
> +    if (dp == NULL) {
> +        snprintf(namebuf, PATH_MAX, "r%s", fname);
> +    } else {
> +        snprintf(namebuf, PATH_MAX, "%.*s/r%s",
> +            (int)(dp - fname), fname, dp + 1);
> +    }
> +    fprintf(stderr, "%s is a block device", fname);
> +    *filename = namebuf;
> +    fprintf(stderr, ", using %s\n", *filename);
> +
> +    return 0;
> +}
> +#else
> +static int raw_normalize_devicepath(const char **filename)
> +{
> +    return 0;
> +}
> +#endif
> +
>   static int raw_open_common(BlockDriverState *bs, const char *filename,
>                              int bdrv_flags, int open_flags)
>   {
>       BDRVRawState *s = bs->opaque;
>       int fd, ret;
> 
> +    ret = raw_normalize_devicepath(&filename);
> +    if (ret != 0) {
> +        return ret;
> +    }
> +
>       s->open_flags = open_flags | O_BINARY;
>       s->open_flags &= ~O_ACCMODE;
>       if (bdrv_flags & BDRV_O_RDWR) {
> 
> 

[Qemu-devel] [PATCH] block/raw-posix: use a character device if a block device is given

[Christoph Egger]

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..64dceb1 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -136,12 +143,55 @@ static int64_t raw_getlength(BlockDriverState *bs);
  static int cdrom_reopen(BlockDriverState *bs);
  #endif

+#if defined(__NetBSD__)
+static int raw_normalize_devicepath(const char **filename)
+{
+    static char namebuf[PATH_MAX];
+    const char *dp, *fname;
+    struct stat sb;
+
+    fname = *filename;
+    dp = strrchr(fname, '/');
+    if (lstat(fname, &sb) < 0) {
+        fprintf(stderr, "%s: stat failed: %s\n",
+            fname, strerror(errno));
+        return -errno;
+    }
+
+    if (!S_ISBLK(sb.st_mode)) {
+        return 0;
+    }
+
+    if (dp == NULL) {
+        snprintf(namebuf, PATH_MAX, "r%s", fname);
+    } else {
+        snprintf(namebuf, PATH_MAX, "%.*s/r%s",
+            (int)(dp - fname), fname, dp + 1);
+    }
+    fprintf(stderr, "%s is a block device", fname);
+    *filename = namebuf;
+    fprintf(stderr, ", using %s\n", *filename);
+
+    return 0;
+}
+#else
+static int raw_normalize_devicepath(const char **filename)
+{
+    return 0;
+}
+#endif
+
  static int raw_open_common(BlockDriverState *bs, const char *filename,
                             int bdrv_flags, int open_flags)
  {
      BDRVRawState *s = bs->opaque;
      int fd, ret;

+    ret = raw_normalize_devicepath(&filename);
+    if (ret != 0) {
+        return ret;
+    }
+
      s->open_flags = open_flags | O_BINARY;
      s->open_flags &= ~O_ACCMODE;
      if (bdrv_flags & BDRV_O_RDWR) {


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632