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

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

[Kevin Wolf]

Kevin Wolf (12):
  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
  nbd: Check against invalid option combinations

 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               | 135 +++++++++++++++++++++++++++++++++-----------
 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       |  30 +++++-----
 24 files changed, 305 insertions(+), 109 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH v2 01/12] 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>
Reviewed-by: Eric Blake <eblake@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 22647b2..0fb0165 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 v2 02/12] 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 56 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index 0fb0165..ef1ae94 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 v2 03/12] 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/qemu/sockets.h |  2 ++
 util/qemu-sockets.c    | 24 ++++++++++++------------
 2 files changed, 14 insertions(+), 12 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..39717d0 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -33,10 +33,10 @@
 
 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),
+/* used temporarily until all users are converted to QemuOpts */
+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 v2 04/12] 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>
Reviewed-by: Eric Blake <eblake@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 39717d0..dc7524d 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 v2 05/12] nbd: Remove unused functions

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@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 v2 06/12] 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>
Reviewed-by: Eric Blake <eblake@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 v2 07/12] 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>
Reviewed-by: Eric Blake <eblake@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 ef1ae94..8a95a28 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 v2 08/12] 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index 8a95a28..2c17a34 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 v2 09/12] 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>
Reviewed-by: Eric Blake <eblake@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 v2 10/12] 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 +++++++++++++++++++++++++++++++++++++++--------
 blockdev.c                | 10 +++++++---
 include/block/block_int.h |  3 +++
 3 files changed, 51 insertions(+), 11 deletions(-)

diff --git a/block.c b/block.c
index 2c17a34..16a92a4 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/blockdev.c b/blockdev.c
index 6f2b759..8cdc9ce 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 v2 11/12] 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/nbd.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 9858f06..67f1df2 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -118,21 +118,18 @@ 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;
-
+        /* nbd[+tcp]://host[:port]/export */
         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:
@@ -223,6 +220,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

[Qemu-devel] [PATCH v2 12/12] nbd: Check against invalid option combinations

[Kevin Wolf]

A file name may only specified if no host or socket path is specified.
The latter two may not appear at the same time either.

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

diff --git a/block/nbd.c b/block/nbd.c
index 67f1df2..3d711b2 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -148,6 +148,15 @@ static void nbd_parse_filename(const char *filename, QDict *options,
     const char *host_spec;
     const char *unixpath;
 
+    if (qdict_haskey(options, "host")
+        || qdict_haskey(options, "port")
+        || qdict_haskey(options, "path"))
+    {
+        error_setg(errp, "host/port/path and a file name may not be specified "
+                         "at the same time");
+        return;
+    }
+
     if (strstr(filename, "://")) {
         int ret = nbd_parse_uri(filename, options);
         if (ret < 0) {
@@ -204,6 +213,11 @@ static int nbd_config(BDRVNBDState *s, QDict *options)
     Error *local_err = NULL;
 
     if (qdict_haskey(options, "path")) {
+        if (qdict_haskey(options, "host")) {
+            qerror_report(ERROR_CLASS_GENERIC_ERROR, "path and host may not "
+                          "be used at the same time.");
+            return -EINVAL;
+        }
         s->is_unix = true;
     } else if (qdict_haskey(options, "host")) {
         s->is_unix = false;
-- 
1.8.1.4

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

[Eric Blake]

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

On 03/20/2013 12:39 PM, 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
> 
> 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 v2 12/12] nbd: Check against invalid option combinations

[Eric Blake]

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

On 03/20/2013 12:39 PM, Kevin Wolf wrote:
> A file name may only specified if no host or socket path is specified.
> The latter two may not appear at the same time either.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/nbd.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)

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

I'm assuming that the rest of the series is unchanged, on the commits
where you carried forward my v1 review.

-- 
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 v2 12/12] nbd: Check against invalid option combinations

[Kevin Wolf]

Am 20.03.2013 um 22:34 hat Eric Blake geschrieben:
> On 03/20/2013 12:39 PM, Kevin Wolf wrote:
> > A file name may only specified if no host or socket path is specified.
> > The latter two may not appear at the same time either.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/nbd.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> I'm assuming that the rest of the series is unchanged, on the commits
> where you carried forward my v1 review.

I think I've added a spelling fix or two for things you pointed out, but
other than that they are unchanged, yes.

Thanks for your review, Eric!

Kevin