block/raw-posix: use a character device if a block device is given

PATCH In review Christoph Egger 3 participants 7 messages May 23, 2011 – May 24, 2011 Block layer

lore

Share & tools

lore

Christoph Egger

author

2011-05-23 12:48 UTC #0 lore raw

PATCH 1/1 block/raw-posix: use a character device if a block device is given

if given a block device, use the character device instead.

From: Manuel Bouyer <[email protected]>

Signed-off-by Christoph Egger <[email protected]>

block/raw-posix.c +34 -0

index 6b72470..d05f373 100644
@@ -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;

Signature · 6 lines

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

Christoph Hellwig

2011-05-23 13:43 UTC #1 lore raw

block/raw-posix.c static int64_t raw_getlength(BlockDriverState *bs); · line 184

Quoted 8 lines

(quoted diff) +2 -0

+    if (lstat(filename, &sb) < 0) {
+        fprintf(stderr, "%s: stat failed: %s\n", filename,

strerror(errno));

block/raw-posix.c +5 -0

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

Christoph Egger

author

2011-05-23 14:01 UTC #2 lore raw

Quoted 10 lines · Christoph Hellwig

block/raw-posix.c static int64_t raw_getlength(BlockDriverState *bs); · line 184
Quoted 8 lines
(quoted diff) +2 -0
+    if (lstat(filename,&sb)<  0) {
+        fprintf(stderr, "%s: stat failed: %s\n", filename,
strerror(errno));
block/raw-posix.c +5 -0
+        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

Quoted 1 line

Also it might be worth to rename it to something like raw_normalize_devicepath.

Signature · 6 lines

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

Kevin Wolf

2011-05-23 14:09 UTC #3 lore raw

In reply to Christoph Egger

Am 23.05.2011 14:34, schrieb Christoph Egger:

Quoted 5 lines

if given a block device, use the character device instead.
From: Manuel Bouyer <[email protected]>
Signed-off-by Christoph Egger <[email protected]>

A useful commit message would explain why you're doing that.

Quoted 14 lines

block/raw-posix.c +5 -0

index 6b72470..d05f373 100644
@@ -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, '/');

block/raw-posix.c static int64_t raw_getlength(BlockDriverState *bs); · line 160

Quoted 10 lines

block/raw-posix.c +10 -0

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

Quoted 10 lines

block/raw-posix.c +10 -0

+
+    return filename;
+}
+#else
+static const char *raw_get_rawdevice(const char *filename)
+{
+    return filename;
+}
+#endif
+

block/raw-posix.c static int64_t raw_getlength(BlockDriverState *bs);

Quoted 10 lines

block/raw-posix.c +4 -0

  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.

block/raw-posix.c static int64_t raw_getlength(BlockDriverState *bs); · line 184

Quoted 5 lines

block/raw-posix.c +5 -0

+        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

Christoph Egger

author

2011-05-24 08:48 UTC #4 lore raw

Quoted 8 lines · Kevin Wolf

Am 23.05.2011 14:34, schrieb Christoph Egger:
Quoted 5 lines
if given a block device, use the character device instead.
From: Manuel Bouyer<[email protected]>
Signed-off-by Christoph Egger<[email protected]>
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.

Quoted 18 lines

block/raw-posix.c +9 -0

index 6b72470..d05f373 100644
@@ -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 {

Quoted 8 lines

block/raw-posix.c static int64_t raw_getlength(BlockDriverState *bs); · line 160
Quoted 6 lines
block/raw-posix.c +6 -0
+        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 ?

Quoted 14 lines

block/raw-posix.c +10 -0

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

Quoted 8 lines

block/raw-posix.c static int64_t raw_getlength(BlockDriverState *bs);
Quoted 6 lines
block/raw-posix.c +4 -0
       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.

Quoted 11 lines

Please remove the fprintf, the callers are responsible for sending an error message to the right destination.
block/raw-posix.c static int64_t raw_getlength(BlockDriverState *bs); · line 184
Quoted 5 lines
block/raw-posix.c +5 -0
+        return -errno;
+    }
+
+    if (S_ISBLK(sb.st_mode))
+        filename = raw_get_rawdevice(filename);

Quoted 4 lines

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

Signature · 6 lines

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

Kevin Wolf

2011-05-24 09:07 UTC #5 lore raw

Am 24.05.2011 10:36, schrieb Christoph Egger:

Quoted 8 lines · Kevin Wolf

Quoted 7 lines
Am 23.05.2011 14:34, schrieb Christoph Egger:
Quoted 5 lines
if given a block device, use the character device instead.
From: Manuel Bouyer<[email protected]>
Signed-off-by Christoph Egger<[email protected]>

Quoted 8 lines

Quoted 1 line
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.

Quoted 26 lines

Quoted 17 lines

block/raw-posix.c +9 -0

index 6b72470..d05f373 100644
@@ -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 {

Quoted 8 lines

block/raw-posix.c static int64_t raw_getlength(BlockDriverState *bs); · line 160
Quoted 6 lines
block/raw-posix.c +6 -0
+        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.

Quoted 2 lines

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

Christoph Egger

author

2011-05-24 09:36 UTC #6 lore raw

Quoted 13 lines · Kevin Wolf

Am 24.05.2011 10:36, schrieb Christoph Egger:
Quoted 12 lines · Kevin Wolf
Quoted 8 lines
Am 23.05.2011 14:34, schrieb Christoph Egger:
Quoted 5 lines
if given a block device, use the character device instead.
From: Manuel Bouyer<[email protected]>
Signed-off-by Christoph Egger<[email protected]>
A useful commit message would explain why you're doing that.
How about this:

Quoted 7 lines

Quoted 4 lines
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.

Quoted 38 lines

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.
Quoted 26 lines
Quoted 17 lines
Quoted 17 lines
block/raw-posix.c +9 -0
index 6b72470..d05f373 100644
@@ -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 {
Quoted 8 lines
block/raw-posix.c static int64_t raw_getlength(BlockDriverState *bs); · line 160
Quoted 6 lines
block/raw-posix.c +6 -0
+        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.
Quoted 2 lines
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

Signature · 6 lines

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