[Qemu-devel] [PATCH 00/11] block: Driver-specific options for protocols

[Qemu-devel] [PATCH 00/11] block: Driver-specific options for protocols

[Kevin Wolf]

With this series applied...

* driver-specific command line options can be passed to protocols

* the protocol driver can be explicitly specified instead of being parsed from
  the file name; the file name can be left out if the protocol doesn't need it

* a new .bdrv_parse_filename callback parses filenames into option QDicts that
  are used for the real bdrv_open()

* NBD makes use of all of these feature so that you can now specify:
    qemu-system-x86_64 -drive file.driver=nbd,file.host=localhost

Kevin Wolf (11):
  block: Add options QDict to bdrv_file_open() prototypes
  block: Pass bdrv_file_open() options to block drivers
  qemu-socket: Make socket_optslist public
  nbd: Keep hostname and port separate
  nbd: Remove unused functions
  nbd: Accept -drive options for the network connection
  block: Introduce .bdrv_parse_filename callback
  block: Rename variable to avoid shadowing
  block: Make find_image_format safe with NULL filename
  block: Allow omitting the file name when using driver-specific options
  nbd: Use default port if only host is specified

 block.c                   | 139 ++++++++++++++++++++++++++++++++++++++++------
 block/blkdebug.c          |   5 +-
 block/blkverify.c         |   5 +-
 block/cow.c               |   2 +-
 block/curl.c              |   3 +-
 block/dmg.c               |  13 ++++-
 block/gluster.c           |   2 +-
 block/iscsi.c             |   5 +-
 block/nbd.c               | 118 ++++++++++++++++++++++++++++-----------
 block/qcow.c              |   2 +-
 block/qcow2.c             |   2 +-
 block/qed.c               |   2 +-
 block/raw-posix.c         |  15 +++--
 block/sheepdog.c          |   7 ++-
 block/vmdk.c              |   2 +-
 block/vvfat.c             |   3 +-
 blockdev.c                |  10 +++-
 include/block/block.h     |   3 +-
 include/block/block_int.h |   7 ++-
 include/block/nbd.h       |   4 +-
 include/qemu/sockets.h    |   3 +
 nbd.c                     |  13 +----
 qemu-io.c                 |   2 +-
 util/qemu-sockets.c       |  28 +++++-----
 24 files changed, 288 insertions(+), 107 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH 01/11] block: Add options QDict to bdrv_file_open() prototypes

[Kevin Wolf]

The new parameter is unused yet.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c                   | 14 +++++++++++---
 block/blkdebug.c          |  5 +++--
 block/blkverify.c         |  5 +++--
 block/cow.c               |  2 +-
 block/curl.c              |  3 ++-
 block/gluster.c           |  2 +-
 block/iscsi.c             |  5 +++--
 block/nbd.c               |  3 ++-
 block/qcow.c              |  2 +-
 block/qcow2.c             |  2 +-
 block/qed.c               |  2 +-
 block/raw-posix.c         | 15 ++++++++++-----
 block/sheepdog.c          |  7 ++++---
 block/vmdk.c              |  2 +-
 block/vvfat.c             |  3 ++-
 include/block/block.h     |  3 ++-
 include/block/block_int.h |  3 ++-
 qemu-io.c                 |  2 +-
 18 files changed, 51 insertions(+), 29 deletions(-)

diff --git a/block.c b/block.c
index 0a062c9..5f4859e 100644
--- a/block.c
+++ b/block.c
@@ -708,7 +708,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
             bdrv_swap(file, bs);
             ret = 0;
         } else {
-            ret = drv->bdrv_file_open(bs, filename, open_flags);
+            ret = drv->bdrv_file_open(bs, filename, options, open_flags);
         }
     } else {
         assert(file != NULL);
@@ -742,13 +742,21 @@ free_and_fail:
 
 /*
  * Opens a file using a protocol (file, host_device, nbd, ...)
+ *
+ * options is a QDict of options to pass to the block drivers, or NULL for an
+ * empty set of options. The reference to the QDict belongs to the block layer
+ * after the call (even on failure), so if the caller intends to reuse the
+ * dictionary, it needs to use QINCREF() before calling bdrv_file_open.
  */
-int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags)
+int bdrv_file_open(BlockDriverState **pbs, const char *filename,
+                   QDict *options, int flags)
 {
     BlockDriverState *bs;
     BlockDriver *drv;
     int ret;
 
+    QDECREF(options);
+
     drv = bdrv_find_protocol(filename);
     if (!drv) {
         return -ENOENT;
@@ -888,7 +896,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
         flags |= BDRV_O_ALLOW_RDWR;
     }
 
-    ret = bdrv_file_open(&file, filename, bdrv_open_flags(bs, flags));
+    ret = bdrv_file_open(&file, filename, NULL, bdrv_open_flags(bs, flags));
     if (ret < 0) {
         goto fail;
     }
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 6f74637..37cfbc7 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -304,7 +304,8 @@ fail:
 }
 
 /* Valid blkdebug filenames look like blkdebug:path/to/config:path/to/image */
-static int blkdebug_open(BlockDriverState *bs, const char *filename, int flags)
+static int blkdebug_open(BlockDriverState *bs, const char *filename,
+                         QDict *options, int flags)
 {
     BDRVBlkdebugState *s = bs->opaque;
     int ret;
@@ -335,7 +336,7 @@ static int blkdebug_open(BlockDriverState *bs, const char *filename, int flags)
     s->state = 1;
 
     /* Open the backing file */
-    ret = bdrv_file_open(&bs->file, filename, flags);
+    ret = bdrv_file_open(&bs->file, filename, NULL, flags);
     if (ret < 0) {
         return ret;
     }
diff --git a/block/blkverify.c b/block/blkverify.c
index 2086d97..59e3b05 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -69,7 +69,8 @@ static void GCC_FMT_ATTR(2, 3) blkverify_err(BlkverifyAIOCB *acb,
 }
 
 /* Valid blkverify filenames look like blkverify:path/to/raw_image:path/to/image */
-static int blkverify_open(BlockDriverState *bs, const char *filename, int flags)
+static int blkverify_open(BlockDriverState *bs, const char *filename,
+                          QDict *options, int flags)
 {
     BDRVBlkverifyState *s = bs->opaque;
     int ret;
@@ -89,7 +90,7 @@ static int blkverify_open(BlockDriverState *bs, const char *filename, int flags)
 
     raw = g_strdup(filename);
     raw[c - filename] = '\0';
-    ret = bdrv_file_open(&bs->file, raw, flags);
+    ret = bdrv_file_open(&bs->file, raw, NULL, flags);
     g_free(raw);
     if (ret < 0) {
         return ret;
diff --git a/block/cow.c b/block/cow.c
index d73e08c..9f94599 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -279,7 +279,7 @@ static int cow_create(const char *filename, QEMUOptionParameter *options)
         return ret;
     }
 
-    ret = bdrv_file_open(&cow_bs, filename, BDRV_O_RDWR);
+    ret = bdrv_file_open(&cow_bs, filename, NULL, BDRV_O_RDWR);
     if (ret < 0) {
         return ret;
     }
diff --git a/block/curl.c b/block/curl.c
index 98947da..186e3b0 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -335,7 +335,8 @@ static void curl_clean_state(CURLState *s)
     s->in_use = 0;
 }
 
-static int curl_open(BlockDriverState *bs, const char *filename, int flags)
+static int curl_open(BlockDriverState *bs, const char *filename,
+                     QDict *options, int flags)
 {
     BDRVCURLState *s = bs->opaque;
     CURLState *state = NULL;
diff --git a/block/gluster.c b/block/gluster.c
index ccd684d..9ccd4d4 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -283,7 +283,7 @@ static int qemu_gluster_aio_flush_cb(void *opaque)
 }
 
 static int qemu_gluster_open(BlockDriverState *bs, const char *filename,
-    int bdrv_flags)
+    QDict *options, int bdrv_flags)
 {
     BDRVGlusterState *s = bs->opaque;
     int open_flags = O_BINARY;
diff --git a/block/iscsi.c b/block/iscsi.c
index 3d52921..51a2889 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1007,7 +1007,8 @@ out:
  * We support iscsi url's on the form
  * iscsi://[<username>%<password>@]<host>[:<port>]/<targetname>/<lun>
  */
-static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
+static int iscsi_open(BlockDriverState *bs, const char *filename,
+                      QDict *options, int flags)
 {
     IscsiLun *iscsilun = bs->opaque;
     struct iscsi_context *iscsi = NULL;
@@ -1203,7 +1204,7 @@ static int iscsi_create(const char *filename, QEMUOptionParameter *options)
     bs.opaque = g_malloc0(sizeof(struct IscsiLun));
     iscsilun = bs.opaque;
 
-    ret = iscsi_open(&bs, filename, 0);
+    ret = iscsi_open(&bs, filename, NULL, 0);
     if (ret != 0) {
         goto out;
     }
diff --git a/block/nbd.c b/block/nbd.c
index a581294..0473908 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -376,7 +376,8 @@ static void nbd_teardown_connection(BlockDriverState *bs)
     closesocket(s->sock);
 }
 
-static int nbd_open(BlockDriverState *bs, const char* filename, int flags)
+static int nbd_open(BlockDriverState *bs, const char* filename,
+                    QDict *options, int flags)
 {
     BDRVNBDState *s = bs->opaque;
     int result;
diff --git a/block/qcow.c b/block/qcow.c
index f6750a5..13d396b 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -679,7 +679,7 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options)
         return ret;
     }
 
-    ret = bdrv_file_open(&qcow_bs, filename, BDRV_O_RDWR);
+    ret = bdrv_file_open(&qcow_bs, filename, NULL, BDRV_O_RDWR);
     if (ret < 0) {
         return ret;
     }
diff --git a/block/qcow2.c b/block/qcow2.c
index 98bb7f3..8ea696a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1254,7 +1254,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
         return ret;
     }
 
-    ret = bdrv_file_open(&bs, filename, BDRV_O_RDWR);
+    ret = bdrv_file_open(&bs, filename, NULL, BDRV_O_RDWR);
     if (ret < 0) {
         return ret;
     }
diff --git a/block/qed.c b/block/qed.c
index 46e12b3..4651403 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -558,7 +558,7 @@ static int qed_create(const char *filename, uint32_t cluster_size,
         return ret;
     }
 
-    ret = bdrv_file_open(&bs, filename, BDRV_O_RDWR | BDRV_O_CACHE_WB);
+    ret = bdrv_file_open(&bs, filename, NULL, BDRV_O_RDWR | BDRV_O_CACHE_WB);
     if (ret < 0) {
         return ret;
     }
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 8a3cdbc..99ac869 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -303,7 +303,8 @@ static int raw_open_common(BlockDriverState *bs, const char *filename,
     return 0;
 }
 
-static int raw_open(BlockDriverState *bs, const char *filename, int flags)
+static int raw_open(BlockDriverState *bs, const char *filename,
+                    QDict *options, int flags)
 {
     BDRVRawState *s = bs->opaque;
 
@@ -1292,7 +1293,8 @@ static int check_hdev_writable(BDRVRawState *s)
     return 0;
 }
 
-static int hdev_open(BlockDriverState *bs, const char *filename, int flags)
+static int hdev_open(BlockDriverState *bs, const char *filename,
+                     QDict *options, int flags)
 {
     BDRVRawState *s = bs->opaque;
     int ret;
@@ -1530,7 +1532,8 @@ static BlockDriver bdrv_host_device = {
 };
 
 #ifdef __linux__
-static int floppy_open(BlockDriverState *bs, const char *filename, int flags)
+static int floppy_open(BlockDriverState *bs, const char *filename,
+                       QDict *options, int flags)
 {
     BDRVRawState *s = bs->opaque;
     int ret;
@@ -1652,7 +1655,8 @@ static BlockDriver bdrv_host_floppy = {
     .bdrv_eject         = floppy_eject,
 };
 
-static int cdrom_open(BlockDriverState *bs, const char *filename, int flags)
+static int cdrom_open(BlockDriverState *bs, const char *filename,
+                      QDict *options, int flags)
 {
     BDRVRawState *s = bs->opaque;
 
@@ -1760,7 +1764,8 @@ static BlockDriver bdrv_host_cdrom = {
 #endif /* __linux__ */
 
 #if defined (__FreeBSD__) || defined(__FreeBSD_kernel__)
-static int cdrom_open(BlockDriverState *bs, const char *filename, int flags)
+static int cdrom_open(BlockDriverState *bs, const char *filename,
+                      QDict *options, int flags)
 {
     BDRVRawState *s = bs->opaque;
     int ret;
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 54d3e53..bb67c4c 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1126,7 +1126,8 @@ static int write_object(int fd, char *buf, uint64_t oid, int copies,
                              create, cache_flags);
 }
 
-static int sd_open(BlockDriverState *bs, const char *filename, int flags)
+static int sd_open(BlockDriverState *bs, const char *filename,
+                   QDict *options, int flags)
 {
     int ret, fd;
     uint32_t vid = 0;
@@ -1269,7 +1270,7 @@ static int sd_prealloc(const char *filename)
     void *buf = g_malloc0(SD_DATA_OBJ_SIZE);
     int ret;
 
-    ret = bdrv_file_open(&bs, filename, BDRV_O_RDWR);
+    ret = bdrv_file_open(&bs, filename, NULL, BDRV_O_RDWR);
     if (ret < 0) {
         goto out;
     }
@@ -1367,7 +1368,7 @@ static int sd_create(const char *filename, QEMUOptionParameter *options)
             goto out;
         }
 
-        ret = bdrv_file_open(&bs, backing_file, 0);
+        ret = bdrv_file_open(&bs, backing_file, NULL, 0);
         if (ret < 0) {
             goto out;
         }
diff --git a/block/vmdk.c b/block/vmdk.c
index e92104a..7bad757 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -661,7 +661,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
 
         path_combine(extent_path, sizeof(extent_path),
                 desc_file_path, fname);
-        ret = bdrv_file_open(&extent_file, extent_path, bs->open_flags);
+        ret = bdrv_file_open(&extent_file, extent_path, NULL, bs->open_flags);
         if (ret) {
             return ret;
         }
diff --git a/block/vvfat.c b/block/vvfat.c
index b8eb38a..ef74c30 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -988,7 +988,8 @@ static void vvfat_rebind(BlockDriverState *bs)
     s->bs = bs;
 }
 
-static int vvfat_open(BlockDriverState *bs, const char* dirname, int flags)
+static int vvfat_open(BlockDriverState *bs, const char* dirname,
+                      QDict *options, int flags)
 {
     BDRVVVFATState *s = bs->opaque;
     int i, cyls, heads, secs;
diff --git a/include/block/block.h b/include/block/block.h
index d4f34d6..9dc6aad 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -135,7 +135,8 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
 void bdrv_delete(BlockDriverState *bs);
 int bdrv_parse_cache_flags(const char *mode, int *flags);
 int bdrv_parse_discard_flags(const char *mode, int *flags);
-int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags);
+int bdrv_file_open(BlockDriverState **pbs, const char *filename,
+                   QDict *options, int flags);
 int bdrv_open_backing_file(BlockDriverState *bs);
 int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
               int flags, BlockDriver *drv);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index ce0aa26..fb2a136 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -83,7 +83,8 @@ struct BlockDriver {
     void (*bdrv_reopen_abort)(BDRVReopenState *reopen_state);
 
     int (*bdrv_open)(BlockDriverState *bs, QDict *options, int flags);
-    int (*bdrv_file_open)(BlockDriverState *bs, const char *filename, int flags);
+    int (*bdrv_file_open)(BlockDriverState *bs, const char *filename,
+                          QDict *options, int flags);
     int (*bdrv_read)(BlockDriverState *bs, int64_t sector_num,
                      uint8_t *buf, int nb_sectors);
     int (*bdrv_write)(BlockDriverState *bs, int64_t sector_num,
diff --git a/qemu-io.c b/qemu-io.c
index 79be516..475a8bd 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -1766,7 +1766,7 @@ static int openfile(char *name, int flags, int growable)
     }
 
     if (growable) {
-        if (bdrv_file_open(&bs, name, flags)) {
+        if (bdrv_file_open(&bs, name, NULL, flags)) {
             fprintf(stderr, "%s: can't open device %s\n", progname, name);
             return 1;
         }
-- 
1.8.1.4

[Qemu-devel] [PATCH 02/11] block: Pass bdrv_file_open() options to block drivers

[Kevin Wolf]

Specify -drive file.option=... on the command line to pass the option to
the protocol instead of the format driver.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 56 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index 5f4859e..9bc9bf3 100644
--- a/block.c
+++ b/block.c
@@ -676,7 +676,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
 
     assert(drv != NULL);
     assert(bs->file == NULL);
-    assert(options == NULL || bs->options != options);
+    assert(options != NULL && bs->options != options);
 
     trace_bdrv_open_common(bs, filename, flags, drv->format_name);
 
@@ -755,22 +755,48 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
     BlockDriver *drv;
     int ret;
 
-    QDECREF(options);
-
     drv = bdrv_find_protocol(filename);
     if (!drv) {
+        QDECREF(options);
         return -ENOENT;
     }
 
+    /* NULL means an empty set of options */
+    if (options == NULL) {
+        options = qdict_new();
+    }
+
     bs = bdrv_new("");
-    ret = bdrv_open_common(bs, NULL, filename, NULL, flags, drv);
+    bs->options = options;
+    options = qdict_clone_shallow(options);
+
+    ret = bdrv_open_common(bs, NULL, filename, options, flags, drv);
     if (ret < 0) {
-        bdrv_delete(bs);
-        return ret;
+        goto fail;
+    }
+
+    /* Check if any unknown options were used */
+    if (qdict_size(options) != 0) {
+        const QDictEntry *entry = qdict_first(options);
+        qerror_report(ERROR_CLASS_GENERIC_ERROR, "Block protocol '%s' doesn't "
+                      "support the option '%s'",
+                      drv->format_name, entry->key);
+        ret = -EINVAL;
+        goto fail;
     }
+    QDECREF(options);
+
     bs->growable = 1;
     *pbs = bs;
     return 0;
+
+fail:
+    QDECREF(options);
+    if (!bs->drv) {
+        QDECREF(bs->options);
+    }
+    bdrv_delete(bs);
+    return ret;
 }
 
 int bdrv_open_backing_file(BlockDriverState *bs)
@@ -810,6 +836,25 @@ int bdrv_open_backing_file(BlockDriverState *bs)
     return 0;
 }
 
+static void extract_subqdict(QDict *src, QDict **dst, const char *start)
+{
+    const QDictEntry *entry, *next;
+    const char *p;
+
+    *dst = qdict_new();
+    entry = qdict_first(src);
+
+    while (entry != NULL) {
+        next = qdict_next(src, entry);
+        if (strstart(entry->key, start, &p)) {
+            qobject_incref(entry->value);
+            qdict_put_obj(*dst, p, entry->value);
+            qdict_del(src, entry->key);
+        }
+        entry = next;
+    }
+}
+
 /*
  * Opens a disk image (raw, qcow2, vmdk, ...)
  *
@@ -825,6 +870,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
     /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
     char tmp_filename[PATH_MAX + 1];
     BlockDriverState *file = NULL;
+    QDict *file_options = NULL;
 
     /* NULL means an empty set of options */
     if (options == NULL) {
@@ -896,7 +942,10 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
         flags |= BDRV_O_ALLOW_RDWR;
     }
 
-    ret = bdrv_file_open(&file, filename, NULL, bdrv_open_flags(bs, flags));
+    extract_subqdict(options, &file_options, "file.");
+
+    ret = bdrv_file_open(&file, filename, file_options,
+                         bdrv_open_flags(bs, flags));
     if (ret < 0) {
         goto fail;
     }
-- 
1.8.1.4

[Qemu-devel] [PATCH 03/11] qemu-socket: Make socket_optslist public

[Kevin Wolf]

Allow other users to create the QemuOpts needed for inet_connect_opts().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/qemu/sockets.h |  2 ++
 util/qemu-sockets.c    | 22 +++++++++++-----------
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index ae5c21c..21846f9 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -30,6 +30,8 @@ int inet_aton(const char *cp, struct in_addr *ia);
 #include "qapi/error.h"
 #include "qapi/qmp/qerror.h"
 
+extern QemuOptsList socket_optslist;
+
 /* misc helpers */
 int qemu_socket(int domain, int type, int protocol);
 int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen);
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 83e4e08..5964e0f 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -34,9 +34,9 @@
 static const int on=1, off=0;
 
 /* used temporarely until all users are converted to QemuOpts */
-static QemuOptsList dummy_opts = {
-    .name = "dummy",
-    .head = QTAILQ_HEAD_INITIALIZER(dummy_opts.head),
+QemuOptsList socket_optslist = {
+    .name = "socket",
+    .head = QTAILQ_HEAD_INITIALIZER(socket_optslist.head),
     .desc = {
         {
             .name = "path",
@@ -583,7 +583,7 @@ int inet_listen(const char *str, char *ostr, int olen,
 
     addr = inet_parse(str, errp);
     if (addr != NULL) {
-        opts = qemu_opts_create_nofail(&dummy_opts);
+        opts = qemu_opts_create_nofail(&socket_optslist);
         inet_addr_to_opts(opts, addr);
         qapi_free_InetSocketAddress(addr);
         sock = inet_listen_opts(opts, port_offset, errp);
@@ -656,7 +656,7 @@ int inet_nonblocking_connect(const char *str,
 
     addr = inet_parse(str, errp);
     if (addr != NULL) {
-        opts = qemu_opts_create_nofail(&dummy_opts);
+        opts = qemu_opts_create_nofail(&socket_optslist);
         inet_addr_to_opts(opts, addr);
         qapi_free_InetSocketAddress(addr);
         sock = inet_connect_opts(opts, errp, callback, opaque);
@@ -799,7 +799,7 @@ int unix_listen(const char *str, char *ostr, int olen, Error **errp)
     char *path, *optstr;
     int sock, len;
 
-    opts = qemu_opts_create_nofail(&dummy_opts);
+    opts = qemu_opts_create_nofail(&socket_optslist);
 
     optstr = strchr(str, ',');
     if (optstr) {
@@ -827,7 +827,7 @@ int unix_connect(const char *path, Error **errp)
     QemuOpts *opts;
     int sock;
 
-    opts = qemu_opts_create_nofail(&dummy_opts);
+    opts = qemu_opts_create_nofail(&socket_optslist);
     qemu_opt_set(opts, "path", path);
     sock = unix_connect_opts(opts, errp, NULL, NULL);
     qemu_opts_del(opts);
@@ -844,7 +844,7 @@ int unix_nonblocking_connect(const char *path,
 
     g_assert(callback != NULL);
 
-    opts = qemu_opts_create_nofail(&dummy_opts);
+    opts = qemu_opts_create_nofail(&socket_optslist);
     qemu_opt_set(opts, "path", path);
     sock = unix_connect_opts(opts, errp, callback, opaque);
     qemu_opts_del(opts);
@@ -895,7 +895,7 @@ int socket_connect(SocketAddress *addr, Error **errp,
     QemuOpts *opts;
     int fd;
 
-    opts = qemu_opts_create_nofail(&dummy_opts);
+    opts = qemu_opts_create_nofail(&socket_optslist);
     switch (addr->kind) {
     case SOCKET_ADDRESS_KIND_INET:
         inet_addr_to_opts(opts, addr->inet);
@@ -926,7 +926,7 @@ int socket_listen(SocketAddress *addr, Error **errp)
     QemuOpts *opts;
     int fd;
 
-    opts = qemu_opts_create_nofail(&dummy_opts);
+    opts = qemu_opts_create_nofail(&socket_optslist);
     switch (addr->kind) {
     case SOCKET_ADDRESS_KIND_INET:
         inet_addr_to_opts(opts, addr->inet);
@@ -954,7 +954,7 @@ int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp)
     QemuOpts *opts;
     int fd;
 
-    opts = qemu_opts_create_nofail(&dummy_opts);
+    opts = qemu_opts_create_nofail(&socket_optslist);
     switch (remote->kind) {
     case SOCKET_ADDRESS_KIND_INET:
         qemu_opt_set(opts, "host", remote->inet->host);
-- 
1.8.1.4

[Qemu-devel] [PATCH 04/11] nbd: Keep hostname and port separate

[Kevin Wolf]

The NBD block supports an URL syntax, for which a URL parser returns
separate hostname and port fields. It also supports the traditional qemu
syntax encoded in a filename. Until now, after parsing the URL to get
each piece of information, a new string is built to be fed to socket
functions.

Instead of building a string in the URL case that is immediately parsed
again, parse the string in both cases and use the QemuOpts interface to
qemu-sockets.c.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/nbd.c            | 49 ++++++++++++++++++++++++++++++++++++++++---------
 include/block/nbd.h    |  2 ++
 include/qemu/sockets.h |  1 +
 nbd.c                  | 12 ++++++++++++
 util/qemu-sockets.c    |  6 +++---
 5 files changed, 58 insertions(+), 12 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 0473908..ecbc892 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -66,7 +66,10 @@ typedef struct BDRVNBDState {
     struct nbd_reply reply;
 
     int is_unix;
-    char *host_spec;
+    char *unix_path;
+
+    InetSocketAddress *inet_addr;
+
     char *export_name; /* An NBD server may export several devices */
 } BDRVNBDState;
 
@@ -112,7 +115,7 @@ static int nbd_parse_uri(BDRVNBDState *s, const char *filename)
             ret = -EINVAL;
             goto out;
         }
-        s->host_spec = g_strdup(qp->p[0].value);
+        s->unix_path = g_strdup(qp->p[0].value);
     } else {
         /* nbd[+tcp]://host:port/export */
         if (!uri->server) {
@@ -122,7 +125,12 @@ static int nbd_parse_uri(BDRVNBDState *s, const char *filename)
         if (!uri->port) {
             uri->port = NBD_DEFAULT_PORT;
         }
-        s->host_spec = g_strdup_printf("%s:%d", uri->server, uri->port);
+
+        s->inet_addr = g_new0(InetSocketAddress, 1);
+        *s->inet_addr = (InetSocketAddress) {
+            .host   = g_strdup(uri->server),
+            .port   = g_strdup_printf("%d", uri->port),
+        };
     }
 
 out:
@@ -140,6 +148,7 @@ static int nbd_config(BDRVNBDState *s, const char *filename)
     const char *host_spec;
     const char *unixpath;
     int err = -EINVAL;
+    Error *local_err = NULL;
 
     if (strstr(filename, "://")) {
         return nbd_parse_uri(s, filename);
@@ -165,10 +174,15 @@ static int nbd_config(BDRVNBDState *s, const char *filename)
     /* are we a UNIX or TCP socket? */
     if (strstart(host_spec, "unix:", &unixpath)) {
         s->is_unix = true;
-        s->host_spec = g_strdup(unixpath);
+        s->unix_path = g_strdup(unixpath);
     } else {
         s->is_unix = false;
-        s->host_spec = g_strdup(host_spec);
+        s->inet_addr = inet_parse(host_spec, &local_err);
+        if (local_err != NULL) {
+            qerror_report_err(local_err);
+            error_free(local_err);
+            goto out;
+        }
     }
 
     err = 0;
@@ -177,7 +191,8 @@ out:
     g_free(file);
     if (err != 0) {
         g_free(s->export_name);
-        g_free(s->host_spec);
+        g_free(s->unix_path);
+        qapi_free_InetSocketAddress(s->inet_addr);
     }
     return err;
 }
@@ -328,9 +343,24 @@ static int nbd_establish_connection(BlockDriverState *bs)
     size_t blocksize;
 
     if (s->is_unix) {
-        sock = unix_socket_outgoing(s->host_spec);
+        sock = unix_socket_outgoing(s->unix_path);
     } else {
-        sock = tcp_socket_outgoing_spec(s->host_spec);
+        QemuOpts *opts = qemu_opts_create_nofail(&socket_optslist);
+
+        qemu_opt_set(opts, "host", s->inet_addr->host);
+        qemu_opt_set(opts, "port", s->inet_addr->port);
+        if (s->inet_addr->has_to) {
+            qemu_opt_set_number(opts, "to", s->inet_addr->to);
+        }
+        if (s->inet_addr->has_ipv4) {
+            qemu_opt_set_number(opts, "ipv4", s->inet_addr->ipv4);
+        }
+        if (s->inet_addr->has_ipv6) {
+            qemu_opt_set_number(opts, "ipv6", s->inet_addr->ipv6);
+        }
+
+        sock = tcp_socket_outgoing_opts(opts);
+        qemu_opts_del(opts);
     }
 
     /* Failed to establish connection */
@@ -550,7 +580,8 @@ static void nbd_close(BlockDriverState *bs)
 {
     BDRVNBDState *s = bs->opaque;
     g_free(s->export_name);
-    g_free(s->host_spec);
+    g_free(s->unix_path);
+    qapi_free_InetSocketAddress(s->inet_addr);
 
     nbd_teardown_connection(bs);
 }
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 344f05b..9b52d50 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -22,6 +22,7 @@
 #include <sys/types.h>
 
 #include "qemu-common.h"
+#include "qemu/option.h"
 
 struct nbd_request {
     uint32_t magic;
@@ -64,6 +65,7 @@ int tcp_socket_outgoing(const char *address, uint16_t port);
 int tcp_socket_incoming(const char *address, uint16_t port);
 int tcp_socket_outgoing_spec(const char *address_and_port);
 int tcp_socket_incoming_spec(const char *address_and_port);
+int tcp_socket_outgoing_opts(QemuOpts *opts);
 int unix_socket_outgoing(const char *path);
 int unix_socket_incoming(const char *path);
 
diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 21846f9..d225f6d 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -47,6 +47,7 @@ int recv_all(int fd, void *buf, int len1, bool single_read);
  */
 typedef void NonBlockingConnectHandler(int fd, void *opaque);
 
+InetSocketAddress *inet_parse(const char *str, Error **errp);
 int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp);
 int inet_listen(const char *str, char *ostr, int olen,
                 int socktype, int port_offset, Error **errp);
diff --git a/nbd.c b/nbd.c
index 0698a02..97879ca 100644
--- a/nbd.c
+++ b/nbd.c
@@ -218,6 +218,18 @@ int tcp_socket_outgoing_spec(const char *address_and_port)
     return fd;
 }
 
+int tcp_socket_outgoing_opts(QemuOpts *opts)
+{
+    Error *local_err = NULL;
+    int fd = inet_connect_opts(opts, &local_err, NULL, NULL);
+    if (local_err != NULL) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+    }
+
+    return fd;
+}
+
 int tcp_socket_incoming(const char *address, uint16_t port)
 {
     char address_and_port[128];
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 5964e0f..3969992 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -485,7 +485,7 @@ err:
 }
 
 /* compatibility wrapper */
-static InetSocketAddress *inet_parse(const char *str, Error **errp)
+InetSocketAddress *inet_parse(const char *str, Error **errp)
 {
     InetSocketAddress *addr;
     const char *optstr, *h;
@@ -555,7 +555,7 @@ fail:
     return NULL;
 }
 
-static void inet_addr_to_opts(QemuOpts *opts, InetSocketAddress *addr)
+static void inet_addr_to_opts(QemuOpts *opts, const InetSocketAddress *addr)
 {
     bool ipv4 = addr->ipv4 || !addr->has_ipv4;
     bool ipv6 = addr->ipv6 || !addr->has_ipv6;
@@ -622,7 +622,7 @@ int inet_connect(const char *str, Error **errp)
 
     addr = inet_parse(str, errp);
     if (addr != NULL) {
-        opts = qemu_opts_create_nofail(&dummy_opts);
+        opts = qemu_opts_create_nofail(&socket_optslist);
         inet_addr_to_opts(opts, addr);
         qapi_free_InetSocketAddress(addr);
         sock = inet_connect_opts(opts, errp, NULL, NULL);
-- 
1.8.1.4

[Qemu-devel] [PATCH 05/11] nbd: Remove unused functions

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/nbd.h |  2 --
 nbd.c               | 19 -------------------
 2 files changed, 21 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 9b52d50..0903d7a 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -61,9 +61,7 @@ enum {
 #define NBD_BUFFER_SIZE (1024*1024)
 
 ssize_t nbd_wr_sync(int fd, void *buffer, size_t size, bool do_read);
-int tcp_socket_outgoing(const char *address, uint16_t port);
 int tcp_socket_incoming(const char *address, uint16_t port);
-int tcp_socket_outgoing_spec(const char *address_and_port);
 int tcp_socket_incoming_spec(const char *address_and_port);
 int tcp_socket_outgoing_opts(QemuOpts *opts);
 int unix_socket_outgoing(const char *path);
diff --git a/nbd.c b/nbd.c
index 97879ca..d1a67ee 100644
--- a/nbd.c
+++ b/nbd.c
@@ -199,25 +199,6 @@ static void combine_addr(char *buf, size_t len, const char* address,
     }
 }
 
-int tcp_socket_outgoing(const char *address, uint16_t port)
-{
-    char address_and_port[128];
-    combine_addr(address_and_port, 128, address, port);
-    return tcp_socket_outgoing_spec(address_and_port);
-}
-
-int tcp_socket_outgoing_spec(const char *address_and_port)
-{
-    Error *local_err = NULL;
-    int fd = inet_connect(address_and_port, &local_err);
-
-    if (local_err != NULL) {
-        qerror_report_err(local_err);
-        error_free(local_err);
-    }
-    return fd;
-}
-
 int tcp_socket_outgoing_opts(QemuOpts *opts)
 {
     Error *local_err = NULL;
-- 
1.8.1.4

[Qemu-devel] [PATCH 06/11] nbd: Accept -drive options for the network connection

[Kevin Wolf]

The existing parsers for the file name now parse everything into the
bdrv_open() options QDict. Instead of using these parsers, you can now
directly specify the options on the command line, like this:

    qemu-system-x86_64 -drive file=nbd:,file.port=1234,file.host=::1

Clearly the file=... part could use further improvement, but it's a
start.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/nbd.c | 129 ++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 77 insertions(+), 52 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index ecbc892..5ed8502 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -32,6 +32,8 @@
 #include "block/block_int.h"
 #include "qemu/module.h"
 #include "qemu/sockets.h"
+#include "qapi/qmp/qjson.h"
+#include "qapi/qmp/qint.h"
 
 #include <sys/types.h>
 #include <unistd.h>
@@ -65,20 +67,19 @@ typedef struct BDRVNBDState {
     Coroutine *recv_coroutine[MAX_NBD_REQUESTS];
     struct nbd_reply reply;
 
-    int is_unix;
-    char *unix_path;
-
-    InetSocketAddress *inet_addr;
+    bool is_unix;
+    QemuOpts *socket_opts;
 
     char *export_name; /* An NBD server may export several devices */
 } BDRVNBDState;
 
-static int nbd_parse_uri(BDRVNBDState *s, const char *filename)
+static int nbd_parse_uri(const char *filename, QDict *options)
 {
     URI *uri;
     const char *p;
     QueryParams *qp = NULL;
     int ret = 0;
+    bool is_unix;
 
     uri = uri_parse(filename);
     if (!uri) {
@@ -87,11 +88,11 @@ static int nbd_parse_uri(BDRVNBDState *s, const char *filename)
 
     /* transport */
     if (!strcmp(uri->scheme, "nbd")) {
-        s->is_unix = false;
+        is_unix = false;
     } else if (!strcmp(uri->scheme, "nbd+tcp")) {
-        s->is_unix = false;
+        is_unix = false;
     } else if (!strcmp(uri->scheme, "nbd+unix")) {
-        s->is_unix = true;
+        is_unix = true;
     } else {
         ret = -EINVAL;
         goto out;
@@ -100,24 +101,26 @@ static int nbd_parse_uri(BDRVNBDState *s, const char *filename)
     p = uri->path ? uri->path : "/";
     p += strspn(p, "/");
     if (p[0]) {
-        s->export_name = g_strdup(p);
+        qdict_put(options, "export", qstring_from_str(p));
     }
 
     qp = query_params_parse(uri->query);
-    if (qp->n > 1 || (s->is_unix && !qp->n) || (!s->is_unix && qp->n)) {
+    if (qp->n > 1 || (is_unix && !qp->n) || (!is_unix && qp->n)) {
         ret = -EINVAL;
         goto out;
     }
 
-    if (s->is_unix) {
+    if (is_unix) {
         /* nbd+unix:///export?socket=path */
         if (uri->server || uri->port || strcmp(qp->p[0].name, "socket")) {
             ret = -EINVAL;
             goto out;
         }
-        s->unix_path = g_strdup(qp->p[0].value);
+        qdict_put(options, "path", qstring_from_str(qp->p[0].value));
     } else {
         /* nbd[+tcp]://host:port/export */
+        char *port_str;
+
         if (!uri->server) {
             ret = -EINVAL;
             goto out;
@@ -126,11 +129,10 @@ static int nbd_parse_uri(BDRVNBDState *s, const char *filename)
             uri->port = NBD_DEFAULT_PORT;
         }
 
-        s->inet_addr = g_new0(InetSocketAddress, 1);
-        *s->inet_addr = (InetSocketAddress) {
-            .host   = g_strdup(uri->server),
-            .port   = g_strdup_printf("%d", uri->port),
-        };
+        port_str = g_strdup_printf("%d", uri->port);
+        qdict_put(options, "host", qstring_from_str(uri->server));
+        qdict_put(options, "port", qstring_from_str(port_str));
+        g_free(port_str);
     }
 
 out:
@@ -141,17 +143,17 @@ out:
     return ret;
 }
 
-static int nbd_config(BDRVNBDState *s, const char *filename)
+static int nbd_parse_filename(const char *filename, QDict *options)
 {
     char *file;
     char *export_name;
     const char *host_spec;
     const char *unixpath;
-    int err = -EINVAL;
+    int ret = -EINVAL;
     Error *local_err = NULL;
 
     if (strstr(filename, "://")) {
-        return nbd_parse_uri(s, filename);
+        return nbd_parse_uri(filename, options);
     }
 
     file = g_strdup(filename);
@@ -163,7 +165,8 @@ static int nbd_config(BDRVNBDState *s, const char *filename)
         }
         export_name[0] = 0; /* truncate 'file' */
         export_name += strlen(EN_OPTSTR);
-        s->export_name = g_strdup(export_name);
+
+        qdict_put(options, "export", qstring_from_str(export_name));
     }
 
     /* extract the host_spec - fail if it's not nbd:... */
@@ -171,32 +174,65 @@ static int nbd_config(BDRVNBDState *s, const char *filename)
         goto out;
     }
 
+    if (!*host_spec) {
+        ret = 1;
+        goto out;
+    }
+
     /* are we a UNIX or TCP socket? */
     if (strstart(host_spec, "unix:", &unixpath)) {
-        s->is_unix = true;
-        s->unix_path = g_strdup(unixpath);
+        qdict_put(options, "path", qstring_from_str(unixpath));
     } else {
-        s->is_unix = false;
-        s->inet_addr = inet_parse(host_spec, &local_err);
+        InetSocketAddress *addr = NULL;
+
+        addr = inet_parse(host_spec, &local_err);
         if (local_err != NULL) {
             qerror_report_err(local_err);
             error_free(local_err);
             goto out;
         }
-    }
 
-    err = 0;
+        qdict_put(options, "host", qstring_from_str(addr->host));
+        qdict_put(options, "port", qstring_from_str(addr->port));
+        qapi_free_InetSocketAddress(addr);
+    }
 
+    ret = 1;
 out:
     g_free(file);
-    if (err != 0) {
-        g_free(s->export_name);
-        g_free(s->unix_path);
-        qapi_free_InetSocketAddress(s->inet_addr);
+    return ret;
+}
+
+static int nbd_config(BDRVNBDState *s, QDict *options)
+{
+    Error *local_err = NULL;
+
+    if (qdict_haskey(options, "path")) {
+        s->is_unix = true;
+    } else if (qdict_haskey(options, "host")) {
+        s->is_unix = false;
+    } else {
+        return -EINVAL;
     }
-    return err;
+
+    s->socket_opts = qemu_opts_create_nofail(&socket_optslist);
+
+    qemu_opts_absorb_qdict(s->socket_opts, options, &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+        return -EINVAL;
+    }
+
+    s->export_name = g_strdup(qdict_get_try_str(options, "export"));
+    if (s->export_name) {
+        qdict_del(options, "export");
+    }
+
+    return 0;
 }
 
+
 static void nbd_coroutine_start(BDRVNBDState *s, struct nbd_request *request)
 {
     int i;
@@ -343,24 +379,9 @@ static int nbd_establish_connection(BlockDriverState *bs)
     size_t blocksize;
 
     if (s->is_unix) {
-        sock = unix_socket_outgoing(s->unix_path);
+        sock = unix_socket_outgoing(qemu_opt_get(s->socket_opts, "path"));
     } else {
-        QemuOpts *opts = qemu_opts_create_nofail(&socket_optslist);
-
-        qemu_opt_set(opts, "host", s->inet_addr->host);
-        qemu_opt_set(opts, "port", s->inet_addr->port);
-        if (s->inet_addr->has_to) {
-            qemu_opt_set_number(opts, "to", s->inet_addr->to);
-        }
-        if (s->inet_addr->has_ipv4) {
-            qemu_opt_set_number(opts, "ipv4", s->inet_addr->ipv4);
-        }
-        if (s->inet_addr->has_ipv6) {
-            qemu_opt_set_number(opts, "ipv6", s->inet_addr->ipv6);
-        }
-
-        sock = tcp_socket_outgoing_opts(opts);
-        qemu_opts_del(opts);
+        sock = tcp_socket_outgoing_opts(s->socket_opts);
     }
 
     /* Failed to establish connection */
@@ -416,7 +437,12 @@ static int nbd_open(BlockDriverState *bs, const char* filename,
     qemu_co_mutex_init(&s->free_sema);
 
     /* Pop the config into our state object. Exit if invalid. */
-    result = nbd_config(s, filename);
+    result = nbd_parse_filename(filename, options);
+    if (result < 0) {
+        return result;
+    }
+
+    result = nbd_config(s, options);
     if (result != 0) {
         return result;
     }
@@ -580,8 +606,7 @@ static void nbd_close(BlockDriverState *bs)
 {
     BDRVNBDState *s = bs->opaque;
     g_free(s->export_name);
-    g_free(s->unix_path);
-    qapi_free_InetSocketAddress(s->inet_addr);
+    qemu_opts_del(s->socket_opts);
 
     nbd_teardown_connection(bs);
 }
-- 
1.8.1.4

[Qemu-devel] [PATCH 07/11] block: Introduce .bdrv_parse_filename callback

[Kevin Wolf]

If a driver needs structured data and not just a string, it can provide
a .bdrv_parse_filename callback now that parses the command line string
into separate options. Keeping this separate from .bdrv_open_filename
ensures that the preferred way of directly specifying the options always
works as well if parsing the string works.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c                   | 11 +++++++++++
 block/nbd.c               | 29 +++++++++++++----------------
 include/block/block_int.h |  1 +
 3 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/block.c b/block.c
index 9bc9bf3..c3ab4fb 100644
--- a/block.c
+++ b/block.c
@@ -770,6 +770,17 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
     bs->options = options;
     options = qdict_clone_shallow(options);
 
+    if (drv->bdrv_parse_filename) {
+        Error *local_err = NULL;
+        drv->bdrv_parse_filename(filename, options, &local_err);
+        if (error_is_set(&local_err)) {
+            qerror_report_err(local_err);
+            error_free(local_err);
+            ret = -EINVAL;
+            goto fail;
+        }
+    }
+
     ret = bdrv_open_common(bs, NULL, filename, options, flags, drv);
     if (ret < 0) {
         goto fail;
diff --git a/block/nbd.c b/block/nbd.c
index 5ed8502..9858f06 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -143,17 +143,20 @@ out:
     return ret;
 }
 
-static int nbd_parse_filename(const char *filename, QDict *options)
+static void nbd_parse_filename(const char *filename, QDict *options,
+                               Error **errp)
 {
     char *file;
     char *export_name;
     const char *host_spec;
     const char *unixpath;
-    int ret = -EINVAL;
-    Error *local_err = NULL;
 
     if (strstr(filename, "://")) {
-        return nbd_parse_uri(filename, options);
+        int ret = nbd_parse_uri(filename, options);
+        if (ret < 0) {
+            error_setg(errp, "No valid URL specified");
+        }
+        return;
     }
 
     file = g_strdup(filename);
@@ -171,11 +174,11 @@ static int nbd_parse_filename(const char *filename, QDict *options)
 
     /* extract the host_spec - fail if it's not nbd:... */
     if (!strstart(file, "nbd:", &host_spec)) {
+        error_setg(errp, "File name string for NBD must start with 'nbd:'");
         goto out;
     }
 
     if (!*host_spec) {
-        ret = 1;
         goto out;
     }
 
@@ -185,10 +188,8 @@ static int nbd_parse_filename(const char *filename, QDict *options)
     } else {
         InetSocketAddress *addr = NULL;
 
-        addr = inet_parse(host_spec, &local_err);
-        if (local_err != NULL) {
-            qerror_report_err(local_err);
-            error_free(local_err);
+        addr = inet_parse(host_spec, errp);
+        if (error_is_set(errp)) {
             goto out;
         }
 
@@ -197,10 +198,8 @@ static int nbd_parse_filename(const char *filename, QDict *options)
         qapi_free_InetSocketAddress(addr);
     }
 
-    ret = 1;
 out:
     g_free(file);
-    return ret;
 }
 
 static int nbd_config(BDRVNBDState *s, QDict *options)
@@ -437,11 +436,6 @@ static int nbd_open(BlockDriverState *bs, const char* filename,
     qemu_co_mutex_init(&s->free_sema);
 
     /* Pop the config into our state object. Exit if invalid. */
-    result = nbd_parse_filename(filename, options);
-    if (result < 0) {
-        return result;
-    }
-
     result = nbd_config(s, options);
     if (result != 0) {
         return result;
@@ -622,6 +616,7 @@ static BlockDriver bdrv_nbd = {
     .format_name         = "nbd",
     .protocol_name       = "nbd",
     .instance_size       = sizeof(BDRVNBDState),
+    .bdrv_parse_filename = nbd_parse_filename,
     .bdrv_file_open      = nbd_open,
     .bdrv_co_readv       = nbd_co_readv,
     .bdrv_co_writev      = nbd_co_writev,
@@ -635,6 +630,7 @@ static BlockDriver bdrv_nbd_tcp = {
     .format_name         = "nbd",
     .protocol_name       = "nbd+tcp",
     .instance_size       = sizeof(BDRVNBDState),
+    .bdrv_parse_filename = nbd_parse_filename,
     .bdrv_file_open      = nbd_open,
     .bdrv_co_readv       = nbd_co_readv,
     .bdrv_co_writev      = nbd_co_writev,
@@ -648,6 +644,7 @@ static BlockDriver bdrv_nbd_unix = {
     .format_name         = "nbd",
     .protocol_name       = "nbd+unix",
     .instance_size       = sizeof(BDRVNBDState),
+    .bdrv_parse_filename = nbd_parse_filename,
     .bdrv_file_open      = nbd_open,
     .bdrv_co_readv       = nbd_co_readv,
     .bdrv_co_writev      = nbd_co_writev,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index fb2a136..1b06a11 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -75,6 +75,7 @@ struct BlockDriver {
     int instance_size;
     int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
     int (*bdrv_probe_device)(const char *filename);
+    void (*bdrv_parse_filename)(const char *filename, QDict *options, Error **errp);
 
     /* For handling image reopen for split or non-split files */
     int (*bdrv_reopen_prepare)(BDRVReopenState *reopen_state,
-- 
1.8.1.4

[Qemu-devel] [PATCH 08/11] block: Rename variable to avoid shadowing

[Kevin Wolf]

bdrv_open() uses two different variables called options. Rename one of
them to avoid confusion and to allow the outer one to be accessed
everywhere.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index c3ab4fb..abe6bd5 100644
--- a/block.c
+++ b/block.c
@@ -896,7 +896,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
         BlockDriverState *bs1;
         int64_t total_size;
         BlockDriver *bdrv_qcow2;
-        QEMUOptionParameter *options;
+        QEMUOptionParameter *create_options;
         char backing_filename[PATH_MAX];
 
         /* if snapshot, we create a temporary backing file and open it
@@ -928,17 +928,19 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
         }
 
         bdrv_qcow2 = bdrv_find_format("qcow2");
-        options = parse_option_parameters("", bdrv_qcow2->create_options, NULL);
+        create_options = parse_option_parameters("", bdrv_qcow2->create_options,
+                                                 NULL);
 
-        set_option_parameter_int(options, BLOCK_OPT_SIZE, total_size);
-        set_option_parameter(options, BLOCK_OPT_BACKING_FILE, backing_filename);
+        set_option_parameter_int(create_options, BLOCK_OPT_SIZE, total_size);
+        set_option_parameter(create_options, BLOCK_OPT_BACKING_FILE,
+                             backing_filename);
         if (drv) {
-            set_option_parameter(options, BLOCK_OPT_BACKING_FMT,
+            set_option_parameter(create_options, BLOCK_OPT_BACKING_FMT,
                 drv->format_name);
         }
 
-        ret = bdrv_create(bdrv_qcow2, tmp_filename, options);
-        free_option_parameters(options);
+        ret = bdrv_create(bdrv_qcow2, tmp_filename, create_options);
+        free_option_parameters(create_options);
         if (ret < 0) {
             goto fail;
         }
-- 
1.8.1.4

[Qemu-devel] [PATCH 09/11] block: Make find_image_format safe with NULL filename

[Kevin Wolf]

In order to achieve this, the .bdrv_probe callbacks of all drivers must
cope with this. The DMG driver is the only one that bases its decision
on the filename and it needs to be changed.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/dmg.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/block/dmg.c b/block/dmg.c
index c1066df..3141cb5 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -51,9 +51,16 @@ typedef struct BDRVDMGState {
 
 static int dmg_probe(const uint8_t *buf, int buf_size, const char *filename)
 {
-    int len=strlen(filename);
-    if(len>4 && !strcmp(filename+len-4,".dmg"))
-	return 2;
+    int len;
+
+    if (!filename) {
+        return 0;
+    }
+
+    len = strlen(filename);
+    if (len > 4 && !strcmp(filename + len - 4, ".dmg")) {
+        return 2;
+    }
     return 0;
 }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 10/11] block: Allow omitting the file name when using driver-specific options

[Kevin Wolf]

After this patch, using -drive with an empty file name continues to open
the file if driver-specific options are used. If no driver-specific
options are specified, the semantics stay as it was: It defines a drive
without an inserted medium.

In order to achieve this, bdrv_open() must be made safe to work with a
NULL filename parameter. The assumption that is made is that only block
drivers which implement bdrv_parse_filename() support using driver
specific options and could therefore work without a filename. These
drivers must make sure to cope with NULL in their implementation of
.bdrv_open() (this is only NBD for now). For all other drivers, the
block layer code will make sure to error out before calling into their
code - they can't possibly work without a filename.

Now an NBD connection can be opened like this:

  qemu-system-x86_64 -drive file.driver=nbd,file.port=1234,file.host=::1

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c                   | 49 +++++++++++++++++++++++++++++++++++++++--------
 block/nbd.c               |  1 -
 blockdev.c                | 10 +++++++---
 include/block/block_int.h |  3 +++
 4 files changed, 51 insertions(+), 12 deletions(-)

diff --git a/block.c b/block.c
index abe6bd5..ae46ca2 100644
--- a/block.c
+++ b/block.c
@@ -688,7 +688,11 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
         bdrv_enable_copy_on_read(bs);
     }
 
-    pstrcpy(bs->filename, sizeof(bs->filename), filename);
+    if (filename != NULL) {
+        pstrcpy(bs->filename, sizeof(bs->filename), filename);
+    } else {
+        bs->filename[0] = '\0';
+    }
 
     if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv)) {
         return -ENOTSUP;
@@ -708,6 +712,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
             bdrv_swap(file, bs);
             ret = 0;
         } else {
+            assert(drv->bdrv_parse_filename || filename != NULL);
             ret = drv->bdrv_file_open(bs, filename, options, open_flags);
         }
     } else {
@@ -727,6 +732,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
 
 #ifndef _WIN32
     if (bs->is_temporary) {
+        assert(filename != NULL);
         unlink(filename);
     }
 #endif
@@ -753,14 +759,9 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
 {
     BlockDriverState *bs;
     BlockDriver *drv;
+    const char *drvname;
     int ret;
 
-    drv = bdrv_find_protocol(filename);
-    if (!drv) {
-        QDECREF(options);
-        return -ENOENT;
-    }
-
     /* NULL means an empty set of options */
     if (options == NULL) {
         options = qdict_new();
@@ -770,7 +771,26 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
     bs->options = options;
     options = qdict_clone_shallow(options);
 
-    if (drv->bdrv_parse_filename) {
+    /* Find the right block driver */
+    drvname = qdict_get_try_str(options, "driver");
+    if (drvname) {
+        drv = bdrv_find_whitelisted_format(drvname);
+        qdict_del(options, "driver");
+    } else if (filename) {
+        drv = bdrv_find_protocol(filename);
+    } else {
+        qerror_report(ERROR_CLASS_GENERIC_ERROR,
+                      "Must specify either driver or file");
+        drv = NULL;
+    }
+
+    if (!drv) {
+        ret = -ENOENT;
+        goto fail;
+    }
+
+    /* Parse the filename and open it */
+    if (drv->bdrv_parse_filename && filename) {
         Error *local_err = NULL;
         drv->bdrv_parse_filename(filename, options, &local_err);
         if (error_is_set(&local_err)) {
@@ -779,6 +799,12 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
             ret = -EINVAL;
             goto fail;
         }
+    } else if (!drv->bdrv_parse_filename && !filename) {
+        qerror_report(ERROR_CLASS_GENERIC_ERROR,
+                      "The '%s' block driver requires a file name",
+                      drv->format_name);
+        ret = -EINVAL;
+        goto fail;
     }
 
     ret = bdrv_open_common(bs, NULL, filename, options, flags, drv);
@@ -899,6 +925,13 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
         QEMUOptionParameter *create_options;
         char backing_filename[PATH_MAX];
 
+        if (qdict_size(options) != 0) {
+            error_report("Can't use snapshot=on with driver-specific options");
+            ret = -EINVAL;
+            goto fail;
+        }
+        assert(filename != NULL);
+
         /* if snapshot, we create a temporary backing file and open it
            instead of opening 'filename' directly */
 
diff --git a/block/nbd.c b/block/nbd.c
index 9858f06..218df6a 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -174,7 +174,6 @@ static void nbd_parse_filename(const char *filename, QDict *options,
 
     /* extract the host_spec - fail if it's not nbd:... */
     if (!strstart(file, "nbd:", &host_spec)) {
-        error_setg(errp, "File name string for NBD must start with 'nbd:'");
         goto out;
     }
 
diff --git a/blockdev.c b/blockdev.c
index 09f76b7..a0b2fb4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -658,7 +658,11 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
         abort();
     }
     if (!file || !*file) {
-        return dinfo;
+        if (qdict_size(bs_opts)) {
+            file = NULL;
+        } else {
+            return dinfo;
+        }
     }
     if (snapshot) {
         /* always use cache=unsafe with snapshot */
@@ -697,10 +701,10 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
     if (ret < 0) {
         if (ret == -EMEDIUMTYPE) {
             error_report("could not open disk image %s: not in %s format",
-                         file, drv->format_name);
+                         file ?: dinfo->id, drv->format_name);
         } else {
             error_report("could not open disk image %s: %s",
-                         file, strerror(-ret));
+                         file ?: dinfo->id, strerror(-ret));
         }
         goto err;
     }
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 1b06a11..0986a2d 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -75,6 +75,9 @@ struct BlockDriver {
     int instance_size;
     int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
     int (*bdrv_probe_device)(const char *filename);
+
+    /* Any driver implementing this callback is expected to be able to handle
+     * NULL file names in its .bdrv_open() implementation */
     void (*bdrv_parse_filename)(const char *filename, QDict *options, Error **errp);
 
     /* For handling image reopen for split or non-split files */
-- 
1.8.1.4

[Qemu-devel] [PATCH 11/11] nbd: Use default port if only host is specified

[Kevin Wolf]

The URL method already takes care to apply the default port when none is
specfied. Directly specifying driver-specific options required the port
number until now. Allow leaving it out and apply the default.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/nbd.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 218df6a..ac61b6d 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -119,20 +119,17 @@ static int nbd_parse_uri(const char *filename, QDict *options)
         qdict_put(options, "path", qstring_from_str(qp->p[0].value));
     } else {
         /* nbd[+tcp]://host:port/export */
-        char *port_str;
-
         if (!uri->server) {
             ret = -EINVAL;
             goto out;
         }
-        if (!uri->port) {
-            uri->port = NBD_DEFAULT_PORT;
-        }
 
-        port_str = g_strdup_printf("%d", uri->port);
         qdict_put(options, "host", qstring_from_str(uri->server));
-        qdict_put(options, "port", qstring_from_str(port_str));
-        g_free(port_str);
+        if (uri->port) {
+            char* port_str = g_strdup_printf("%d", uri->port);
+            qdict_put(options, "port", qstring_from_str(port_str));
+            g_free(port_str);
+        }
     }
 
 out:
@@ -222,6 +219,10 @@ static int nbd_config(BDRVNBDState *s, QDict *options)
         return -EINVAL;
     }
 
+    if (!qemu_opt_get(s->socket_opts, "port")) {
+        qemu_opt_set_number(s->socket_opts, "port", NBD_DEFAULT_PORT);
+    }
+
     s->export_name = g_strdup(qdict_get_try_str(options, "export"));
     if (s->export_name) {
         qdict_del(options, "export");
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 01/11] block: Add options QDict to bdrv_file_open() prototypes

[Eric Blake]

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

On 03/18/2013 11:23 AM, Kevin Wolf wrote:
> The new parameter is unused yet.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---

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: 621 bytes --]

Re: [Qemu-devel] [PATCH 02/11] block: Pass bdrv_file_open() options to block drivers

[Eric Blake]

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

On 03/18/2013 11:23 AM, Kevin Wolf wrote:
> Specify -drive file.option=... on the command line to pass the option to
> the protocol instead of the format driver.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 56 insertions(+), 7 deletions(-)

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

Still hoping we expose this in QMP for hotplug, but I'll see how things
go through the series...

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH 02/11] block: Pass bdrv_file_open() options to block drivers

[Kevin Wolf]

Am 19.03.2013 um 18:48 hat Eric Blake geschrieben:
> On 03/18/2013 11:23 AM, Kevin Wolf wrote:
> > Specify -drive file.option=... on the command line to pass the option to
> > the protocol instead of the format driver.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 56 insertions(+), 7 deletions(-)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> Still hoping we expose this in QMP for hotplug, but I'll see how things
> go through the series...

We'll get there, but I can't do everything at once. So my plan is to
complete the command line extensions first and only then get to QMP. I'm
almost sure that we're thinking of very similar things that we want to
have as the final result, but one step after another.

Kevin

Re: [Qemu-devel] [PATCH 02/11] block: Pass bdrv_file_open() options to block drivers

[Eric Blake]

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

On 03/19/2013 12:05 PM, Kevin Wolf wrote:
> Am 19.03.2013 um 18:48 hat Eric Blake geschrieben:
>> On 03/18/2013 11:23 AM, Kevin Wolf wrote:
>>> Specify -drive file.option=... on the command line to pass the option to
>>> the protocol instead of the format driver.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  block.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
>>>  1 file changed, 56 insertions(+), 7 deletions(-)
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
>> Still hoping we expose this in QMP for hotplug, but I'll see how things
>> go through the series...
> 
> We'll get there, but I can't do everything at once. So my plan is to
> complete the command line extensions first and only then get to QMP. I'm
> almost sure that we're thinking of very similar things that we want to
> have as the final result, but one step after another.

Not a problem - just hoping that we can get there in time for 1.5.  I'm
happy to review these types of patches, since it directly affects
interfaces that libvirt will want to use.

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH 03/11] qemu-socket: Make socket_optslist public

[Eric Blake]

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

On 03/18/2013 11:23 AM, Kevin Wolf wrote:
> Allow other users to create the QemuOpts needed for inet_connect_opts().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/qemu/sockets.h |  2 ++
>  util/qemu-sockets.c    | 22 +++++++++++-----------
>  2 files changed, 13 insertions(+), 11 deletions(-)

> +++ b/util/qemu-sockets.c
> @@ -34,9 +34,9 @@
>  static const int on=1, off=0;
>  
>  /* used temporarely until all users are converted to QemuOpts */

As long as you are in the area,

s/temporarely/temporarily/

That's trivial, though, 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: 621 bytes --]

Re: [Qemu-devel] [PATCH 04/11] nbd: Keep hostname and port separate

[Eric Blake]

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

On 03/18/2013 11:23 AM, Kevin Wolf wrote:
> The NBD block supports an URL syntax, for which a URL parser returns
> separate hostname and port fields. It also supports the traditional qemu
> syntax encoded in a filename. Until now, after parsing the URL to get
> each piece of information, a new string is built to be fed to socket
> functions.
> 
> Instead of building a string in the URL case that is immediately parsed
> again, parse the string in both cases and use the QemuOpts interface to
> qemu-sockets.c.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/nbd.c            | 49 ++++++++++++++++++++++++++++++++++++++++---------
>  include/block/nbd.h    |  2 ++
>  include/qemu/sockets.h |  1 +
>  nbd.c                  | 12 ++++++++++++
>  util/qemu-sockets.c    |  6 +++---
>  5 files changed, 58 insertions(+), 12 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: 621 bytes --]

Re: [Qemu-devel] [PATCH 05/11] nbd: Remove unused functions

[Eric Blake]

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

On 03/18/2013 11:23 AM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/nbd.h |  2 --
>  nbd.c               | 19 -------------------
>  2 files changed, 21 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: 621 bytes --]

Re: [Qemu-devel] [PATCH 06/11] nbd: Accept -drive options for the network connection

[Eric Blake]

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

On 03/18/2013 11:23 AM, Kevin Wolf wrote:
> The existing parsers for the file name now parse everything into the
> bdrv_open() options QDict. Instead of using these parsers, you can now
> directly specify the options on the command line, like this:
> 
>     qemu-system-x86_64 -drive file=nbd:,file.port=1234,file.host=::1
> 
> Clearly the file=... part could use further improvement, but it's a
> start.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/nbd.c | 129 ++++++++++++++++++++++++++++++++++++------------------------
>  1 file changed, 77 insertions(+), 52 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: 621 bytes --]

Re: [Qemu-devel] [PATCH 07/11] block: Introduce .bdrv_parse_filename callback

[Eric Blake]

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

On 03/18/2013 11:23 AM, Kevin Wolf wrote:
> If a driver needs structured data and not just a string, it can provide
> a .bdrv_parse_filename callback now that parses the command line string
> into separate options. Keeping this separate from .bdrv_open_filename
> ensures that the preferred way of directly specifying the options always
> works as well if parsing the string works.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c                   | 11 +++++++++++
>  block/nbd.c               | 29 +++++++++++++----------------
>  include/block/block_int.h |  1 +
>  3 files changed, 25 insertions(+), 16 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: 621 bytes --]

Re: [Qemu-devel] [PATCH 08/11] block: Rename variable to avoid shadowing

[Eric Blake]

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

On 03/18/2013 11:23 AM, Kevin Wolf wrote:
> bdrv_open() uses two different variables called options. Rename one of
> them to avoid confusion and to allow the outer one to be accessed
> everywhere.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)

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

Should configure be setting up -Wshadow to help prevent instances like this?

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH 09/11] block: Make find_image_format safe with NULL filename

[Eric Blake]

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

On 03/18/2013 11:23 AM, Kevin Wolf wrote:
> In order to achieve this, the .bdrv_probe callbacks of all drivers must
> cope with this. The DMG driver is the only one that bases its decision
> on the filename and it needs to be changed.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/dmg.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)

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

> 
> diff --git a/block/dmg.c b/block/dmg.c
> index c1066df..3141cb5 100644
> --- a/block/dmg.c
> +++ b/block/dmg.c
> @@ -51,9 +51,16 @@ typedef struct BDRVDMGState {
>  
>  static int dmg_probe(const uint8_t *buf, int buf_size, const char *filename)
>  {
> -    int len=strlen(filename);
> -    if(len>4 && !strcmp(filename+len-4,".dmg"))

Someone really didn't like whitespace :)  Glad to see it rewritten to
keep the patch checker happy.

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH 10/11] block: Allow omitting the file name when using driver-specific options

[Eric Blake]

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

On 03/18/2013 11:24 AM, Kevin Wolf wrote:
> After this patch, using -drive with an empty file name continues to open
> the file if driver-specific options are used. If no driver-specific
> options are specified, the semantics stay as it was: It defines a drive
> without an inserted medium.
> 
> In order to achieve this, bdrv_open() must be made safe to work with a
> NULL filename parameter. The assumption that is made is that only block
> drivers which implement bdrv_parse_filename() support using driver
> specific options and could therefore work without a filename. These
> drivers must make sure to cope with NULL in their implementation of
> .bdrv_open() (this is only NBD for now). For all other drivers, the
> block layer code will make sure to error out before calling into their
> code - they can't possibly work without a filename.
> 
> Now an NBD connection can be opened like this:
> 
>   qemu-system-x86_64 -drive file.driver=nbd,file.port=1234,file.host=::1

Slick.

> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c                   | 49 +++++++++++++++++++++++++++++++++++++++--------
>  block/nbd.c               |  1 -
>  blockdev.c                | 10 +++++++---
>  include/block/block_int.h |  3 +++
>  4 files changed, 51 insertions(+), 12 deletions(-)

> +++ b/block/nbd.c
> @@ -174,7 +174,6 @@ static void nbd_parse_filename(const char *filename, QDict *options,
>  
>      /* extract the host_spec - fail if it's not nbd:... */
>      if (!strstart(file, "nbd:", &host_spec)) {
> -        error_setg(errp, "File name string for NBD must start with 'nbd:'");
>          goto out;
>      }

Is this really right?  The code allows me to pass both file= and
file.driver= at once; so what if I pass -drive file.driver=nbd,file=xyz.
 Prior to this patch, I'd get a nice message about file=xyz not starting
with nbd:, but now there is a silent failure.

I think it might be better if you keep the error_setg(), and instead
rewrite the if on the previous line:

if (file && !strstart(file, "nbd:", &host_spec)) {

Also, since it looks like the code allows me to pass both file.driver=
and file= at once, if I pass both pieces of information, is there any
sanity checking that the two are identical, and/or should we error out
and declare that if driver options are used then nbd requires a NULL
filename?


> @@ -697,10 +701,10 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
>      if (ret < 0) {
>          if (ret == -EMEDIUMTYPE) {
>              error_report("could not open disk image %s: not in %s format",
> -                         file, drv->format_name);
> +                         file ?: dinfo->id, drv->format_name);

You're not the first person to use this gcc extension, but the more
instances we add, the harder it will be to scrub them back out if
someone ever insists on portability to another compiler.

>          } else {
>              error_report("could not open disk image %s: %s",
> -                         file, strerror(-ret));
> +                         file ?: dinfo->id, strerror(-ret));
>          }
>          goto err;
>      }
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 1b06a11..0986a2d 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -75,6 +75,9 @@ struct BlockDriver {
>      int instance_size;
>      int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
>      int (*bdrv_probe_device)(const char *filename);
> +
> +    /* Any driver implementing this callback is expected to be able to handle
> +     * NULL file names in its .bdrv_open() implementation */
>      void (*bdrv_parse_filename)(const char *filename, QDict *options, Error **errp);
>  
>      /* For handling image reopen for split or non-split files */
> 

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH 11/11] nbd: Use default port if only host is specified

[Eric Blake]

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

On 03/18/2013 11:24 AM, Kevin Wolf wrote:
> The URL method already takes care to apply the default port when none is
> specfied. Directly specifying driver-specific options required the port
> number until now. Allow leaving it out and apply the default.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/nbd.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 218df6a..ac61b6d 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -119,20 +119,17 @@ static int nbd_parse_uri(const char *filename, QDict *options)
>          qdict_put(options, "path", qstring_from_str(qp->p[0].value));
>      } else {
>          /* nbd[+tcp]://host:port/export */

Should this comment be rewritten as:

/* nbd[+tcp]://host[:port]/export */

But that's trivial, 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: 621 bytes --]

Re: [Qemu-devel] [PATCH 09/11] block: Make find_image_format safe with NULL filename

[Kevin Wolf]

Am 20.03.2013 um 03:14 hat Eric Blake geschrieben:
> On 03/18/2013 11:23 AM, Kevin Wolf wrote:
> > In order to achieve this, the .bdrv_probe callbacks of all drivers must
> > cope with this. The DMG driver is the only one that bases its decision
> > on the filename and it needs to be changed.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/dmg.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> > 
> > diff --git a/block/dmg.c b/block/dmg.c
> > index c1066df..3141cb5 100644
> > --- a/block/dmg.c
> > +++ b/block/dmg.c
> > @@ -51,9 +51,16 @@ typedef struct BDRVDMGState {
> >  
> >  static int dmg_probe(const uint8_t *buf, int buf_size, const char *filename)
> >  {
> > -    int len=strlen(filename);
> > -    if(len>4 && !strcmp(filename+len-4,".dmg"))
> 
> Someone really didn't like whitespace :)  Glad to see it rewritten to
> keep the patch checker happy.

Actually I don't think it would have complained, because it would be
context only. But I simply can't leave this code alone while touching
the function. :-)

Kevin

Re: [Qemu-devel] [PATCH 08/11] block: Rename variable to avoid shadowing

[Kevin Wolf]

Am 20.03.2013 um 03:07 hat Eric Blake geschrieben:
> On 03/18/2013 11:23 AM, Kevin Wolf wrote:
> > bdrv_open() uses two different variables called options. Rename one of
> > them to avoid confusion and to allow the outer one to be accessed
> > everywhere.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> Should configure be setting up -Wshadow to help prevent instances like this?

Perhaps it should. But I tried it out and there are by far too many
warnings, so that doing this patch wouldn't be a trivial task.

Kevin

Re: [Qemu-devel] [PATCH 10/11] block: Allow omitting the file name when using driver-specific options

[Kevin Wolf]

Am 20.03.2013 um 03:27 hat Eric Blake geschrieben:
> > +++ b/block/nbd.c
> > @@ -174,7 +174,6 @@ static void nbd_parse_filename(const char *filename, QDict *options,
> >  
> >      /* extract the host_spec - fail if it's not nbd:... */
> >      if (!strstart(file, "nbd:", &host_spec)) {
> > -        error_setg(errp, "File name string for NBD must start with 'nbd:'");
> >          goto out;
> >      }
> 
> Is this really right?  The code allows me to pass both file= and
> file.driver= at once; so what if I pass -drive file.driver=nbd,file=xyz.
>  Prior to this patch, I'd get a nice message about file=xyz not starting
> with nbd:, but now there is a silent failure.
> 
> I think it might be better if you keep the error_setg(), and instead
> rewrite the if on the previous line:
> 
> if (file && !strstart(file, "nbd:", &host_spec)) {

In fact, the additional check isn't even necessary because the function
is only called for file != NULL in the first place. I've put the error
back.

> Also, since it looks like the code allows me to pass both file.driver=
> and file= at once, if I pass both pieces of information, is there any
> sanity checking that the two are identical, and/or should we error out
> and declare that if driver options are used then nbd requires a NULL
> filename?

I've added another patch that checks that only one of host/port/filename
is specified.

Kevin