[Qemu-devel] [PATCH v4 0/5] Some fixes related to scsi-generic

[Qemu-devel] [PATCH v4 0/5] Some fixes related to scsi-generic

[Dimitris Aragiorgis]

Hi all,

These four patches make slight changes to the way QEMU handles SCSI
generic devices to fix a number of small problems.

I am sending them against the master branch, since I don't know if they
can be considered bugfixes.

Thanks,
dimara

v4 (rebased to current master):
* Avoid errno clobbering with DPRINTF + strerror() (Eric's comment)
* Use {} in #define macro even if it is not necessary (single commands)

v3 (rebased to current master):
* Avoid bit-rot in DPRINTF (adopt Eric's suggestion)
* Address Kevin's comments (DEBUF_FLOPPY, line > 80 chars, SG device)
* Mention Kevin's comment wrt disk flush in the corresponding commit

v2:
* remove duplicate check for sg inside iscsi_co_flush()
* remove DEBUG_BLOCK_PRINT in block/raw-posix.c
* use DPRINTF for debugging in block/raw-posix.c

PS: Paolo suggested to use a tracepoint inside hdev_is_sg() but I chose DPRINTF
instead. It would make sense to add a tracepoint for bdrv_is_sg() (just like
most bdrv_* commands) but this is too much for now since it just returns the
bs->sg flag (and is not an actual driver function). If you insist I'll change
it in v3.

Dimitris Aragiorgis (5):
  block: Use bdrv_is_sg() everywhere
  Fix migration in case of scsi-generic
  raw-posix: DPRINTF instead of DEBUG_BLOCK_PRINT
  raw-posix: Use DPRINTF for DEBUG_FLOPPY
  raw-posix: Introduce hdev_is_sg()

 block.c           |    6 ++--
 block/io.c        |    3 +-
 block/iscsi.c     |    4 ---
 block/raw-posix.c |   91 +++++++++++++++++++++++++++++++----------------------
 4 files changed, 58 insertions(+), 46 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH v4 1/5] block: Use bdrv_is_sg() everywhere

[Dimitris Aragiorgis]

Instead of checking bs->sg use bdrv_is_sg() consistently throughout
the code.

Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c           |    6 +++---
 block/iscsi.c     |    2 +-
 block/raw-posix.c |    4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 7904098..d651b57 100644
--- a/block.c
+++ b/block.c
@@ -566,7 +566,7 @@ static int find_image_format(BlockDriverState *bs, const char *filename,
     int ret = 0;
 
     /* Return the raw BlockDriver * to scsi-generic devices or empty drives */
-    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0) {
+    if (bdrv_is_sg(bs) || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0) {
         *pdrv = &bdrv_raw;
         return ret;
     }
@@ -598,7 +598,7 @@ static int refresh_total_sectors(BlockDriverState *bs, int64_t hint)
     BlockDriver *drv = bs->drv;
 
     /* Do not attempt drv->bdrv_getlength() on scsi-generic devices */
-    if (bs->sg)
+    if (bdrv_is_sg(bs))
         return 0;
 
     /* query actual device if possible, otherwise just trust the hint */
@@ -890,7 +890,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
     }
 
     assert(bdrv_opt_mem_align(bs) != 0);
-    assert((bs->request_alignment != 0) || bs->sg);
+    assert((bs->request_alignment != 0) || bdrv_is_sg(bs));
     return 0;
 
 free_and_fail:
diff --git a/block/iscsi.c b/block/iscsi.c
index 8fca1d3..502a81f 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -627,7 +627,7 @@ static int coroutine_fn iscsi_co_flush(BlockDriverState *bs)
     IscsiLun *iscsilun = bs->opaque;
     struct IscsiTask iTask;
 
-    if (bs->sg) {
+    if (bdrv_is_sg(bs)) {
         return 0;
     }
 
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 24d8582..fb58440 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -302,9 +302,9 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
     BDRVRawState *s = bs->opaque;
     char *buf;
 
-    /* For /dev/sg devices the alignment is not really used.
+    /* For SCSI generic devices the alignment is not really used.
        With buffered I/O, we don't have any restrictions. */
-    if (bs->sg || !s->needs_alignment) {
+    if (bdrv_is_sg(bs) || !s->needs_alignment) {
         bs->request_alignment = 1;
         s->buf_align = 1;
         return;
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH v4 1/5] block: Use bdrv_is_sg() everywhere

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 485 bytes --]

On Wed, May 20, 2015 at 12:57:35PM +0300, Dimitris Aragiorgis wrote:
> Instead of checking bs->sg use bdrv_is_sg() consistently throughout
> the code.
> 
> Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block.c           |    6 +++---
>  block/iscsi.c     |    2 +-
>  block/raw-posix.c |    4 ++--
>  3 files changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

[Qemu-devel] [PATCH v4 2/5] Fix migration in case of scsi-generic

[Dimitris Aragiorgis]

During migration, QEMU uses fsync()/fdatasync() on the open file
descriptor for read-write block devices to flush data just before
stopping the VM.

However, fsync() on a scsi-generic device returns -EINVAL which
causes the migration to fail. This patch skips flushing data in case
of an SG device, since submitting SCSI commands directly via an SG
character device (e.g. /dev/sg0) bypasses the page cache completely,
anyway.

Note that fsync() not only flushes the page cache but also the disk
cache. The scsi-generic device never sends flushes, and for
migration it assumes that the same SCSI device is used by the
destination host, so it does not issue any SCSI SYNCHRONIZE CACHE
(10) command.

Finally, remove the bdrv_is_sg() test from iscsi_co_flush() since
this is now redundant (we flush the underlying protocol at the end
of bdrv_co_flush() which, with this patch, we never reach).

Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
---
 block/io.c    |    3 ++-
 block/iscsi.c |    4 ----
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/block/io.c b/block/io.c
index 1ce62c4..922dc07 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2231,7 +2231,8 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
 {
     int ret;
 
-    if (!bs || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
+    if (!bs || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs) ||
+        bdrv_is_sg(bs)) {
         return 0;
     }
 
diff --git a/block/iscsi.c b/block/iscsi.c
index 502a81f..965978b 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -627,10 +627,6 @@ static int coroutine_fn iscsi_co_flush(BlockDriverState *bs)
     IscsiLun *iscsilun = bs->opaque;
     struct IscsiTask iTask;
 
-    if (bdrv_is_sg(bs)) {
-        return 0;
-    }
-
     if (!iscsilun->force_next_flush) {
         return 0;
     }
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH v4 2/5] Fix migration in case of scsi-generic

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1254 bytes --]

On Wed, May 20, 2015 at 12:57:36PM +0300, Dimitris Aragiorgis wrote:
> During migration, QEMU uses fsync()/fdatasync() on the open file
> descriptor for read-write block devices to flush data just before
> stopping the VM.
> 
> However, fsync() on a scsi-generic device returns -EINVAL which
> causes the migration to fail. This patch skips flushing data in case
> of an SG device, since submitting SCSI commands directly via an SG
> character device (e.g. /dev/sg0) bypasses the page cache completely,
> anyway.
> 
> Note that fsync() not only flushes the page cache but also the disk
> cache. The scsi-generic device never sends flushes, and for
> migration it assumes that the same SCSI device is used by the
> destination host, so it does not issue any SCSI SYNCHRONIZE CACHE
> (10) command.
> 
> Finally, remove the bdrv_is_sg() test from iscsi_co_flush() since
> this is now redundant (we flush the underlying protocol at the end
> of bdrv_co_flush() which, with this patch, we never reach).
> 
> Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
> ---
>  block/io.c    |    3 ++-
>  block/iscsi.c |    4 ----
>  2 files changed, 2 insertions(+), 5 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

[Qemu-devel] [PATCH v4 3/5] raw-posix: DPRINTF instead of DEBUG_BLOCK_PRINT

[Dimitris Aragiorgis]

Building the QEMU tools fails if we #define DEBUG_BLOCK inside
block/raw-posix.c. Here instead of adding qemu-log.o in block-obj-y
so that DEBUG_BLOCK_PRINT can be used, we substitute the latter with
a simple DPRINTF() (that does not cause bit-rot).

Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
---
 block/raw-posix.c |   26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index fb58440..438bf0b 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -97,12 +97,18 @@
 //#define DEBUG_FLOPPY
 
 //#define DEBUG_BLOCK
-#if defined(DEBUG_BLOCK)
-#define DEBUG_BLOCK_PRINT(formatCstr, ...) do { if (qemu_log_enabled()) \
-    { qemu_log(formatCstr, ## __VA_ARGS__); qemu_log_flush(); } } while (0)
+
+#ifdef DEBUG_BLOCK
+# define DEBUG_BLOCK_PRINT 1
 #else
-#define DEBUG_BLOCK_PRINT(formatCstr, ...)
+# define DEBUG_BLOCK_PRINT 0
 #endif
+#define DPRINTF(fmt, ...) \
+do { \
+    if (DEBUG_BLOCK_PRINT) { \
+        printf(fmt, ## __VA_ARGS__); \
+    } \
+} while (0)
 
 /* OS X does not have O_DSYNC */
 #ifndef O_DSYNC
@@ -1016,6 +1022,7 @@ static ssize_t handle_aiocb_rw(RawPosixAIOData *aiocb)
 static int xfs_write_zeroes(BDRVRawState *s, int64_t offset, uint64_t bytes)
 {
     struct xfs_flock64 fl;
+    int err;
 
     memset(&fl, 0, sizeof(fl));
     fl.l_whence = SEEK_SET;
@@ -1023,8 +1030,9 @@ static int xfs_write_zeroes(BDRVRawState *s, int64_t offset, uint64_t bytes)
     fl.l_len = bytes;
 
     if (xfsctl(NULL, s->fd, XFS_IOC_ZERO_RANGE, &fl) < 0) {
-        DEBUG_BLOCK_PRINT("cannot write zero range (%s)\n", strerror(errno));
-        return -errno;
+        err = errno;
+        DPRINTF("cannot write zero range (%s)\n", strerror(errno));
+        return -err;
     }
 
     return 0;
@@ -1033,6 +1041,7 @@ static int xfs_write_zeroes(BDRVRawState *s, int64_t offset, uint64_t bytes)
 static int xfs_discard(BDRVRawState *s, int64_t offset, uint64_t bytes)
 {
     struct xfs_flock64 fl;
+    int err;
 
     memset(&fl, 0, sizeof(fl));
     fl.l_whence = SEEK_SET;
@@ -1040,8 +1049,9 @@ static int xfs_discard(BDRVRawState *s, int64_t offset, uint64_t bytes)
     fl.l_len = bytes;
 
     if (xfsctl(NULL, s->fd, XFS_IOC_UNRESVSP64, &fl) < 0) {
-        DEBUG_BLOCK_PRINT("cannot punch hole (%s)\n", strerror(errno));
-        return -errno;
+        err = errno;
+        DPRINTF("cannot punch hole (%s)\n", strerror(errno));
+        return -err;
     }
 
     return 0;
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH v4 3/5] raw-posix: DPRINTF instead of DEBUG_BLOCK_PRINT

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1159 bytes --]

On 05/20/2015 03:57 AM, Dimitris Aragiorgis wrote:
> Building the QEMU tools fails if we #define DEBUG_BLOCK inside
> block/raw-posix.c. Here instead of adding qemu-log.o in block-obj-y
> so that DEBUG_BLOCK_PRINT can be used, we substitute the latter with
> a simple DPRINTF() (that does not cause bit-rot).
> 
> Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
> ---
>  block/raw-posix.c |   26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 

> @@ -1040,8 +1049,9 @@ static int xfs_discard(BDRVRawState *s, int64_t offset, uint64_t bytes)
>      fl.l_len = bytes;
>  
>      if (xfsctl(NULL, s->fd, XFS_IOC_UNRESVSP64, &fl) < 0) {
> -        DEBUG_BLOCK_PRINT("cannot punch hole (%s)\n", strerror(errno));
> -        return -errno;
> +        err = errno;
> +        DPRINTF("cannot punch hole (%s)\n", strerror(errno));
> +        return -err;

Could use strerror(err) to shave two source bytes, but doesn't change
correctness, so:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 3/5] raw-posix: DPRINTF instead of DEBUG_BLOCK_PRINT

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 565 bytes --]

On Wed, May 20, 2015 at 12:57:37PM +0300, Dimitris Aragiorgis wrote:
> Building the QEMU tools fails if we #define DEBUG_BLOCK inside
> block/raw-posix.c. Here instead of adding qemu-log.o in block-obj-y
> so that DEBUG_BLOCK_PRINT can be used, we substitute the latter with
> a simple DPRINTF() (that does not cause bit-rot).
> 
> Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
> ---
>  block/raw-posix.c |   26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

[Qemu-devel] [PATCH v4 4/5] raw-posix: Use DPRINTF for DEBUG_FLOPPY

[Dimitris Aragiorgis]

Get rid of several #ifdef DEBUG_FLOPPY and substitute them with
DPRINTF.

Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
---
 block/raw-posix.c |   22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 438bf0b..ace228f 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -94,8 +94,6 @@
 #include <xfs/xfs.h>
 #endif
 
-//#define DEBUG_FLOPPY
-
 //#define DEBUG_BLOCK
 
 #ifdef DEBUG_BLOCK
@@ -2167,16 +2165,12 @@ static int fd_open(BlockDriverState *bs)
         (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - s->fd_open_time) >= FD_OPEN_TIMEOUT) {
         qemu_close(s->fd);
         s->fd = -1;
-#ifdef DEBUG_FLOPPY
-        printf("Floppy closed\n");
-#endif
+        DPRINTF("Floppy closed\n");
     }
     if (s->fd < 0) {
         if (s->fd_got_error &&
             (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - s->fd_error_time) < FD_OPEN_TIMEOUT) {
-#ifdef DEBUG_FLOPPY
-            printf("No floppy (open delayed)\n");
-#endif
+            DPRINTF("No floppy (open delayed)\n");
             return -EIO;
         }
         s->fd = qemu_open(bs->filename, s->open_flags & ~O_NONBLOCK);
@@ -2185,14 +2179,10 @@ static int fd_open(BlockDriverState *bs)
             s->fd_got_error = 1;
             if (last_media_present)
                 s->fd_media_changed = 1;
-#ifdef DEBUG_FLOPPY
-            printf("No floppy\n");
-#endif
+            DPRINTF("No floppy\n");
             return -EIO;
         }
-#ifdef DEBUG_FLOPPY
-        printf("Floppy opened\n");
-#endif
+        DPRINTF("Floppy opened\n");
     }
     if (!last_media_present)
         s->fd_media_changed = 1;
@@ -2460,9 +2450,7 @@ static int floppy_media_changed(BlockDriverState *bs)
     fd_open(bs);
     ret = s->fd_media_changed;
     s->fd_media_changed = 0;
-#ifdef DEBUG_FLOPPY
-    printf("Floppy changed=%d\n", ret);
-#endif
+    DPRINTF("Floppy changed=%d\n", ret);
     return ret;
 }
 
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH v4 4/5] raw-posix: Use DPRINTF for DEBUG_FLOPPY

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 464 bytes --]

On 05/20/2015 03:57 AM, Dimitris Aragiorgis wrote:
> Get rid of several #ifdef DEBUG_FLOPPY and substitute them with
> DPRINTF.
> 
> Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
> ---
>  block/raw-posix.c |   22 +++++-----------------
>  1 file changed, 5 insertions(+), 17 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 4/5] raw-posix: Use DPRINTF for DEBUG_FLOPPY

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 378 bytes --]

On Wed, May 20, 2015 at 12:57:38PM +0300, Dimitris Aragiorgis wrote:
> Get rid of several #ifdef DEBUG_FLOPPY and substitute them with
> DPRINTF.
> 
> Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
> ---
>  block/raw-posix.c |   22 +++++-----------------
>  1 file changed, 5 insertions(+), 17 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

[Qemu-devel] [PATCH v4 5/5] raw-posix: Introduce hdev_is_sg()

[Dimitris Aragiorgis]

Until now, an SG device was identified only by checking if its path
started with "/dev/sg". Then, hdev_open() set bs->sg accordingly.
This is very fragile, e.g. it fails with symlinks or relative paths.
We should rely on the actual properties of the device instead of the
specified file path.

Test for an SG device (e.g. /dev/sg0) by ensuring that all of the
following holds:

 - The device supports the SG_GET_VERSION_NUM ioctl
 - The device supports the SG_GET_SCSI_ID ioctl
 - The specified file name corresponds to a character device

Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
---
 block/raw-posix.c |   39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index ace228f..d84e5a0 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -57,6 +57,7 @@
 #include <linux/fd.h>
 #include <linux/fs.h>
 #include <linux/hdreg.h>
+#include <scsi/sg.h>
 #ifdef __s390__
 #include <asm/dasd.h>
 #endif
@@ -2081,15 +2082,38 @@ static void hdev_parse_filename(const char *filename, QDict *options,
     qdict_put_obj(options, "filename", QOBJECT(qstring_from_str(filename)));
 }
 
+static int hdev_is_sg(BlockDriverState *bs)
+{
+
+#if defined(__linux__)
+
+    struct stat st;
+    struct sg_scsi_id scsiid;
+    int sg_version;
+
+    if (!bdrv_ioctl(bs, SG_GET_VERSION_NUM, &sg_version) &&
+        !bdrv_ioctl(bs, SG_GET_SCSI_ID, &scsiid) &&
+        stat(bs->filename, &st) >= 0 && S_ISCHR(st.st_mode)) {
+        DPRINTF("SG device found: type=%d, version=%d\n",
+            scsiid.scsi_type, sg_version);
+        return 1;
+    }
+
+#endif
+
+    return 0;
+}
+
 static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
                      Error **errp)
 {
     BDRVRawState *s = bs->opaque;
     Error *local_err = NULL;
     int ret;
-    const char *filename = qdict_get_str(options, "filename");
 
 #if defined(__APPLE__) && defined(__MACH__)
+    const char *filename = qdict_get_str(options, "filename");
+
     if (strstart(filename, "/dev/cdrom", NULL)) {
         kern_return_t kernResult;
         io_iterator_t mediaIterator;
@@ -2118,16 +2142,6 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
 #endif
 
     s->type = FTYPE_FILE;
-#if defined(__linux__)
-    {
-        char resolved_path[ MAXPATHLEN ], *temp;
-
-        temp = realpath(filename, resolved_path);
-        if (temp && strstart(temp, "/dev/sg", NULL)) {
-            bs->sg = 1;
-        }
-    }
-#endif
 
     ret = raw_open_common(bs, options, flags, 0, &local_err);
     if (ret < 0) {
@@ -2137,6 +2151,9 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
         return ret;
     }
 
+    /* Since this does ioctl the device must be already opened */
+    bs->sg = hdev_is_sg(bs);
+
     if (flags & BDRV_O_RDWR) {
         ret = check_hdev_writable(s);
         if (ret < 0) {
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH v4 5/5] raw-posix: Introduce hdev_is_sg()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1476 bytes --]

On 05/20/2015 03:57 AM, Dimitris Aragiorgis wrote:
> Until now, an SG device was identified only by checking if its path
> started with "/dev/sg". Then, hdev_open() set bs->sg accordingly.
> This is very fragile, e.g. it fails with symlinks or relative paths.
> We should rely on the actual properties of the device instead of the
> specified file path.
> 
> Test for an SG device (e.g. /dev/sg0) by ensuring that all of the
> following holds:
> 
>  - The device supports the SG_GET_VERSION_NUM ioctl
>  - The device supports the SG_GET_SCSI_ID ioctl
>  - The specified file name corresponds to a character device

I'd check stat first. ioctl's on the wrong device type might have
unexpected side effects.


> +static int hdev_is_sg(BlockDriverState *bs)
> +{
> +
> +#if defined(__linux__)
> +
> +    struct stat st;
> +    struct sg_scsi_id scsiid;
> +    int sg_version;
> +
> +    if (!bdrv_ioctl(bs, SG_GET_VERSION_NUM, &sg_version) &&
> +        !bdrv_ioctl(bs, SG_GET_SCSI_ID, &scsiid) &&
> +        stat(bs->filename, &st) >= 0 && S_ISCHR(st.st_mode)) {
> +        DPRINTF("SG device found: type=%d, version=%d\n",
> +            scsiid.scsi_type, sg_version);
> +        return 1;
> +    }
> +
> +#endif
> +
> +    return 0;

Make this return 'bool', using 'true' and 'false'.  We require a C99
compiler, after all.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 5/5] raw-posix: Introduce hdev_is_sg()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 267 bytes --]

On Wed, May 20, 2015 at 12:57:39PM +0300, Dimitris Aragiorgis wrote:
> This is very fragile, e.g. it fails with symlinks or relative paths.

This is not true since realpath(3) is used to resolve symlinks and
product an absolute path.

Is this patch really necessary?

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v4 5/5] raw-posix: Introduce hdev_is_sg()

[Dimitris Aragiorgis]

[-- Attachment #1: Type: text/plain, Size: 2020 bytes --]

Hello Stefan,

Yes, you are right. Using realpath() is a workaround for supporting
symlinks, as long as they point to a path starting with "/dev/sg". I
will remove this reference in the revised version of this patch.

However, it still holds that determining whether a filename is an SG
device or not is based on textual (and hardcoded) examination, i.e.,
whether it starts with "/dev/sg".

The point is that a device node is a device node no matter what its
filename or its location in the fs tree is. A device node with  major 1,
minor 3 is the null device whether it's called /dev/null or not.

The proposed patch allows the use of any device node which is capable of
acting as a scsi-generic device; it checks for the required intrinsic
property ("does this device support the SG ioctl?") versus trying to
second-guess the nature of the device node by checking its filename.

Similarly, the programming example from SG's documentation does not
check the node of the device node at all:
http://sg.danny.cz/sg/p/sg_v3_ho.html#pexample

    /* It is prudent to check we have a sg device by trying an ioctl */
    if ((ioctl(sg_fd, SG_GET_VERSION_NUM, &k) < 0) || (k < 30000)) {
        printf("%s is not an sg device, or old sg driver\n", argv[1]);
        return 1;

The above has the advantage that it works with device nodes of any name,
and in any directory.

So I suggest we go with the submitted patch taking into account Eric's
proposal: The code first stat()s the given filename to ensure it is a
character device node, and then it issues the SG_GET_VERSION_NUM and
SG_GET_SCSI_ID ioctl()s.

Thanks,
dimara

* Stefan Hajnoczi <stefanha@redhat.com> [2015-06-19 13:33:02 +0100]:

> On Wed, May 20, 2015 at 12:57:39PM +0300, Dimitris Aragiorgis wrote:
> > This is very fragile, e.g. it fails with symlinks or relative paths.
> 
> This is not true since realpath(3) is used to resolve symlinks and
> product an absolute path.
> 
> Is this patch really necessary?



[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v4 5/5] raw-posix: Introduce hdev_is_sg()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 531 bytes --]

On Mon, Jun 22, 2015 at 01:18:43PM +0300, Dimitris Aragiorgis wrote:
> So I suggest we go with the submitted patch taking into account Eric's
> proposal: The code first stat()s the given filename to ensure it is a
> character device node, and then it issues the SG_GET_VERSION_NUM and
> SG_GET_SCSI_ID ioctl()s.

That's fine.

Regarding Eric's comment about ioctl number collisions, I've checked
Linux Documentation/ioctl/ioctl-number.txt.  At least under Linux there
is no collision for SG_GET_VERSION_NUM/SG_GET_SCSI_ID.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v4 0/5] Some fixes related to scsi-generic

[Dimitris Aragiorgis]

[-- Attachment #1: Type: text/plain, Size: 1860 bytes --]

Hi,

* Dimitris Aragiorgis <dimara@arrikto.com> [2015-05-20 12:57:34 +0300]:

> Hi all,
> 
> These four patches make slight changes to the way QEMU handles SCSI
> generic devices to fix a number of small problems.
> 
> I am sending them against the master branch, since I don't know if they
> can be considered bugfixes.
> 
> Thanks,
> dimara
> 
> v4 (rebased to current master):
> * Avoid errno clobbering with DPRINTF + strerror() (Eric's comment)
> * Use {} in #define macro even if it is not necessary (single commands)
> 

Any news on this?

Thanks,
dimara


> v3 (rebased to current master):
> * Avoid bit-rot in DPRINTF (adopt Eric's suggestion)
> * Address Kevin's comments (DEBUF_FLOPPY, line > 80 chars, SG device)
> * Mention Kevin's comment wrt disk flush in the corresponding commit
> 
> v2:
> * remove duplicate check for sg inside iscsi_co_flush()
> * remove DEBUG_BLOCK_PRINT in block/raw-posix.c
> * use DPRINTF for debugging in block/raw-posix.c
> 
> PS: Paolo suggested to use a tracepoint inside hdev_is_sg() but I chose DPRINTF
> instead. It would make sense to add a tracepoint for bdrv_is_sg() (just like
> most bdrv_* commands) but this is too much for now since it just returns the
> bs->sg flag (and is not an actual driver function). If you insist I'll change
> it in v3.
> 
> Dimitris Aragiorgis (5):
>   block: Use bdrv_is_sg() everywhere
>   Fix migration in case of scsi-generic
>   raw-posix: DPRINTF instead of DEBUG_BLOCK_PRINT
>   raw-posix: Use DPRINTF for DEBUG_FLOPPY
>   raw-posix: Introduce hdev_is_sg()
> 
>  block.c           |    6 ++--
>  block/io.c        |    3 +-
>  block/iscsi.c     |    4 ---
>  block/raw-posix.c |   91 +++++++++++++++++++++++++++++++----------------------
>  4 files changed, 58 insertions(+), 46 deletions(-)
> 
> -- 
> 1.7.10.4
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [Qemu-devel] [PATCH v4 0/5] Some fixes related to scsi-generic

[Dimitris Aragiorgis]

[-- Attachment #1: Type: text/plain, Size: 2171 bytes --]

Hi,

did you have the time to take a look at this? Is there anything
pending from my side?

Thanks,
dimara

* Dimitris Aragiorgis <dimara@arrikto.com> [2015-05-29 08:15:34 +0300]:

> Hi,
> 
> * Dimitris Aragiorgis <dimara@arrikto.com> [2015-05-20 12:57:34 +0300]:
> 
> > Hi all,
> > 
> > These four patches make slight changes to the way QEMU handles SCSI
> > generic devices to fix a number of small problems.
> > 
> > I am sending them against the master branch, since I don't know if they
> > can be considered bugfixes.
> > 
> > Thanks,
> > dimara
> > 
> > v4 (rebased to current master):
> > * Avoid errno clobbering with DPRINTF + strerror() (Eric's comment)
> > * Use {} in #define macro even if it is not necessary (single commands)
> > 
> 
> Any news on this?
> 
> Thanks,
> dimara
> 
> 
> > v3 (rebased to current master):
> > * Avoid bit-rot in DPRINTF (adopt Eric's suggestion)
> > * Address Kevin's comments (DEBUF_FLOPPY, line > 80 chars, SG device)
> > * Mention Kevin's comment wrt disk flush in the corresponding commit
> > 
> > v2:
> > * remove duplicate check for sg inside iscsi_co_flush()
> > * remove DEBUG_BLOCK_PRINT in block/raw-posix.c
> > * use DPRINTF for debugging in block/raw-posix.c
> > 
> > PS: Paolo suggested to use a tracepoint inside hdev_is_sg() but I chose DPRINTF
> > instead. It would make sense to add a tracepoint for bdrv_is_sg() (just like
> > most bdrv_* commands) but this is too much for now since it just returns the
> > bs->sg flag (and is not an actual driver function). If you insist I'll change
> > it in v3.
> > 
> > Dimitris Aragiorgis (5):
> >   block: Use bdrv_is_sg() everywhere
> >   Fix migration in case of scsi-generic
> >   raw-posix: DPRINTF instead of DEBUG_BLOCK_PRINT
> >   raw-posix: Use DPRINTF for DEBUG_FLOPPY
> >   raw-posix: Introduce hdev_is_sg()
> > 
> >  block.c           |    6 ++--
> >  block/io.c        |    3 +-
> >  block/iscsi.c     |    4 ---
> >  block/raw-posix.c |   91 +++++++++++++++++++++++++++++++----------------------
> >  4 files changed, 58 insertions(+), 46 deletions(-)
> > 
> > -- 
> > 1.7.10.4
> > 



[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]