[Qemu-devel] [PATCH 00/15] block: A bunch of fixes for Sheepdog and Gluster

[Qemu-devel] [PATCH 00/15] block: A bunch of fixes for Sheepdog and Gluster

[Markus Armbruster]

Bad error handling, memory leaks, and lack of blockdev-add support.

Markus Armbruster (15):
  sheepdog: Defuse time bomb in sd_open() error handling
  sheepdog: Fix error handling in sd_snapshot_delete()
  sheepdog: Fix error handling sd_create()
  sheepdog: Mark sd_snapshot_delete() lossage FIXME
  sheepdog: Fix snapshot ID parsing in _open(), _create, _goto()
  sheepdog: Don't truncate long VDI name in _open(), _create()
  sheepdog: Report errors in pseudo-filename more usefully
  sheepdog: Use SocketAddress and socket_connect()
  sheepdog: Implement bdrv_parse_filename()
  gluster: Drop assumptions on SocketTransport names
  gluster: Don't duplicate qapi-util.c's qapi_enum_parse()
  gluster: Plug memory leaks in qemu_gluster_parse_json()
  qapi-schema: Rename GlusterServer to SocketAddressFlat
  qapi-schema: Rename SocketAddressFlat's variant tcp to inet
  sheepdog: Support blockdev-add

 block/gluster.c      | 127 +++++++--------
 block/sheepdog.c     | 436 +++++++++++++++++++++++++++++++++++++--------------
 qapi-schema.json     |  38 +++++
 qapi/block-core.json |  73 +++------
 4 files changed, 443 insertions(+), 231 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 01/15] sheepdog: Defuse time bomb in sd_open() error handling

[Markus Armbruster]

When qemu_opts_absorb_qdict() fails, sd_open() closes stdin, because
sd->fd is still zero.  Fortunately, qemu_opts_absorb_qdict() can't
fail, because:

1. it only fails when qemu_opt_parse() fails, and
2. the only member of runtime_opts.desc[] is a QEMU_OPT_STRING, and
3. qemu_opt_parse() can't fail for QEMU_OPT_STRING.

Defuse this ticking time bomb by jumping behind the file descriptor
cleanup on error.

Also do that for the error paths where sd->fd is still -1.  The file
descriptor cleanup happens to do nothing then, but let's not rely on
that here.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/sheepdog.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 860ba61..fe15723 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1392,7 +1392,7 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
     if (local_err) {
         error_propagate(errp, local_err);
         ret = -EINVAL;
-        goto out;
+        goto out_no_fd;
     }
 
     filename = qemu_opt_get(opts, "filename");
@@ -1412,12 +1412,12 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
     }
     if (ret < 0) {
         error_setg(errp, "Can't parse filename");
-        goto out;
+        goto out_no_fd;
     }
     s->fd = get_sheep_fd(s, errp);
     if (s->fd < 0) {
         ret = s->fd;
-        goto out;
+        goto out_no_fd;
     }
 
     ret = find_vdi_name(s, vdi, snapid, tag, &vid, true, errp);
@@ -1472,6 +1472,7 @@ out:
     if (s->fd >= 0) {
         closesocket(s->fd);
     }
+out_no_fd:
     qemu_opts_del(opts);
     g_free(buf);
     return ret;
-- 
2.7.4

Re: [Qemu-devel] [PATCH 01/15] sheepdog: Defuse time bomb in sd_open() error handling

[Eric Blake]

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

On 03/02/2017 03:43 PM, Markus Armbruster wrote:
> When qemu_opts_absorb_qdict() fails, sd_open() closes stdin, because
> sd->fd is still zero.  Fortunately, qemu_opts_absorb_qdict() can't
> fail, because:
> 
> 1. it only fails when qemu_opt_parse() fails, and
> 2. the only member of runtime_opts.desc[] is a QEMU_OPT_STRING, and
> 3. qemu_opt_parse() can't fail for QEMU_OPT_STRING.
> 
> Defuse this ticking time bomb by jumping behind the file descriptor
> cleanup on error.
> 
> Also do that for the error paths where sd->fd is still -1.  The file
> descriptor cleanup happens to do nothing then, but let's not rely on
> that here.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/sheepdog.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 

>      s->fd = get_sheep_fd(s, errp);
>      if (s->fd < 0) {
>          ret = s->fd;
> -        goto out;
> +        goto out_no_fd;
>      }

Thanks to this change...

>  
>      ret = find_vdi_name(s, vdi, snapid, tag, &vid, true, errp);
> @@ -1472,6 +1472,7 @@ out:
>      if (s->fd >= 0) {

...this 'if' is now always true, and you can unconditionally call
closesocket().

For that matter, the 'out:' label is a bit of a misnomer, since it is
only reached on errors.

>          closesocket(s->fd);
>      }
> +out_no_fd:
>      qemu_opts_del(opts);
>      g_free(buf);
>      return ret;
> 

The cleanup is correct, but looks like it can be more extensive.

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


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

Re: [Qemu-devel] [PATCH 01/15] sheepdog: Defuse time bomb in sd_open() error handling

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/02/2017 03:43 PM, Markus Armbruster wrote:
>> When qemu_opts_absorb_qdict() fails, sd_open() closes stdin, because
>> sd->fd is still zero.  Fortunately, qemu_opts_absorb_qdict() can't
>> fail, because:
>> 
>> 1. it only fails when qemu_opt_parse() fails, and
>> 2. the only member of runtime_opts.desc[] is a QEMU_OPT_STRING, and
>> 3. qemu_opt_parse() can't fail for QEMU_OPT_STRING.
>> 
>> Defuse this ticking time bomb by jumping behind the file descriptor
>> cleanup on error.
>> 
>> Also do that for the error paths where sd->fd is still -1.  The file
>> descriptor cleanup happens to do nothing then, but let's not rely on
>> that here.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/sheepdog.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>> 
>
>>      s->fd = get_sheep_fd(s, errp);
>>      if (s->fd < 0) {
>>          ret = s->fd;
>> -        goto out;
>> +        goto out_no_fd;
>>      }
>
> Thanks to this change...
>
>>  
>>      ret = find_vdi_name(s, vdi, snapid, tag, &vid, true, errp);
>> @@ -1472,6 +1472,7 @@ out:
>>      if (s->fd >= 0) {
>
> ...this 'if' is now always true, and you can unconditionally call
> closesocket().

Yup.

close(-1) is just fine anyway.

> For that matter, the 'out:' label is a bit of a misnomer, since it is
> only reached on errors.
>
>>          closesocket(s->fd);
>>      }
>> +out_no_fd:
>>      qemu_opts_del(opts);
>>      g_free(buf);
>>      return ret;
>> 
>
> The cleanup is correct, but looks like it can be more extensive.

I'll have another look at it.

[Qemu-devel] [PATCH 02/15] sheepdog: Fix error handling in sd_snapshot_delete()

[Markus Armbruster]

As a bdrv_snapshot_delete() method, sd_snapshot_delete() must set an
error and return negative errno on failure.  It sometimes returns -1,
and sometimes neglects to set an error.  It also prints error messages
with error_report().  Fix all that.

Moreover, its handling of an attempt to delete an nonexistent snapshot
is wrong: it error_report()s and succeeds.  Fix it to set an error and
return -ENOENT instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/sheepdog.c | 37 +++++++++++++++++--------------------
 1 file changed, 17 insertions(+), 20 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index fe15723..e4e5345 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2406,18 +2406,15 @@ out:
 
 #define NR_BATCHED_DISCARD 128
 
-static bool remove_objects(BDRVSheepdogState *s)
+static int remove_objects(BDRVSheepdogState *s, Error **errp)
 {
     int fd, i = 0, nr_objs = 0;
-    Error *local_err = NULL;
     int ret = 0;
-    bool result = true;
     SheepdogInode *inode = &s->inode;
 
-    fd = connect_to_sdog(s, &local_err);
+    fd = connect_to_sdog(s, errp);
     if (fd < 0) {
-        error_report_err(local_err);
-        return false;
+        return fd;
     }
 
     nr_objs = count_data_objs(inode);
@@ -2447,15 +2444,14 @@ static bool remove_objects(BDRVSheepdogState *s)
                                     data_vdi_id[start_idx]),
                            false, s->cache_flags);
         if (ret < 0) {
-            error_report("failed to discard snapshot inode.");
-            result = false;
+            error_setg(errp, "failed to discard snapshot inode.");
             goto out;
         }
     }
 
 out:
     closesocket(fd);
-    return result;
+    return ret;
 }
 
 static int sd_snapshot_delete(BlockDriverState *bs,
@@ -2465,7 +2461,6 @@ static int sd_snapshot_delete(BlockDriverState *bs,
 {
     unsigned long snap_id = 0;
     char snap_tag[SD_MAX_VDI_TAG_LEN];
-    Error *local_err = NULL;
     int fd, ret;
     char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN];
     BDRVSheepdogState *s = bs->opaque;
@@ -2478,8 +2473,9 @@ static int sd_snapshot_delete(BlockDriverState *bs,
     };
     SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr;
 
-    if (!remove_objects(s)) {
-        return -1;
+    ret = remove_objects(s, errp);
+    if (ret) {
+        return ret;
     }
 
     memset(buf, 0, sizeof(buf));
@@ -2500,35 +2496,36 @@ static int sd_snapshot_delete(BlockDriverState *bs,
     }
 
     ret = find_vdi_name(s, s->name, snap_id, snap_tag, &vid, true,
-                        &local_err);
+                        errp);
     if (ret) {
         return ret;
     }
 
-    fd = connect_to_sdog(s, &local_err);
+    fd = connect_to_sdog(s, errp);
     if (fd < 0) {
-        error_report_err(local_err);
-        return -1;
+        return fd;
     }
 
     ret = do_req(fd, s->bs, (SheepdogReq *)&hdr,
                  buf, &wlen, &rlen);
     closesocket(fd);
     if (ret) {
+        error_setg_errno(errp, -ret, "Couldn't send request to server");
         return ret;
     }
 
     switch (rsp->result) {
     case SD_RES_NO_VDI:
-        error_report("%s was already deleted", s->name);
+        error_setg(errp, "Can't find the snapshot");
+        return -ENOENT;
     case SD_RES_SUCCESS:
         break;
     default:
-        error_report("%s, %s", sd_strerror(rsp->result), s->name);
-        return -1;
+        error_setg(errp, "%s", sd_strerror(rsp->result));
+        return -EIO;
     }
 
-    return ret;
+    return 0;
 }
 
 static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
-- 
2.7.4

Re: [Qemu-devel] [PATCH 02/15] sheepdog: Fix error handling in sd_snapshot_delete()

[Eric Blake]

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

On 03/02/2017 03:43 PM, Markus Armbruster wrote:
> As a bdrv_snapshot_delete() method, sd_snapshot_delete() must set an
> error and return negative errno on failure.  It sometimes returns -1,
> and sometimes neglects to set an error.  It also prints error messages
> with error_report().  Fix all that.
> 
> Moreover, its handling of an attempt to delete an nonexistent snapshot
> is wrong: it error_report()s and succeeds.  Fix it to set an error and
> return -ENOENT instead.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/sheepdog.c | 37 +++++++++++++++++--------------------
>  1 file changed, 17 insertions(+), 20 deletions(-)
> 

> @@ -2447,15 +2444,14 @@ static bool remove_objects(BDRVSheepdogState *s)
>                                      data_vdi_id[start_idx]),
>                             false, s->cache_flags);
>          if (ret < 0) {
> -            error_report("failed to discard snapshot inode.");
> -            result = false;
> +            error_setg(errp, "failed to discard snapshot inode.");

Lose the trailing '.'

With that fixed,
Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [PATCH 02/15] sheepdog: Fix error handling in sd_snapshot_delete()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/02/2017 03:43 PM, Markus Armbruster wrote:
>> As a bdrv_snapshot_delete() method, sd_snapshot_delete() must set an
>> error and return negative errno on failure.  It sometimes returns -1,
>> and sometimes neglects to set an error.  It also prints error messages
>> with error_report().  Fix all that.
>> 
>> Moreover, its handling of an attempt to delete an nonexistent snapshot
>> is wrong: it error_report()s and succeeds.  Fix it to set an error and
>> return -ENOENT instead.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/sheepdog.c | 37 +++++++++++++++++--------------------
>>  1 file changed, 17 insertions(+), 20 deletions(-)
>> 
>
>> @@ -2447,15 +2444,14 @@ static bool remove_objects(BDRVSheepdogState *s)
>>                                      data_vdi_id[start_idx]),
>>                             false, s->cache_flags);
>>          if (ret < 0) {
>> -            error_report("failed to discard snapshot inode.");
>> -            result = false;
>> +            error_setg(errp, "failed to discard snapshot inode.");
>
> Lose the trailing '.'

Good idea.

> With that fixed,
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH 02/15] sheepdog: Fix error handling in sd_snapshot_delete()

[Kevin Wolf]

Am 02.03.2017 um 22:43 hat Markus Armbruster geschrieben:
> As a bdrv_snapshot_delete() method, sd_snapshot_delete() must set an
> error and return negative errno on failure.  It sometimes returns -1,
> and sometimes neglects to set an error.  It also prints error messages
> with error_report().  Fix all that.
> 
> Moreover, its handling of an attempt to delete an nonexistent snapshot
> is wrong: it error_report()s and succeeds.  Fix it to set an error and
> return -ENOENT instead.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

> -static bool remove_objects(BDRVSheepdogState *s)
> +static int remove_objects(BDRVSheepdogState *s, Error **errp)
>  {
>      int fd, i = 0, nr_objs = 0;
> -    Error *local_err = NULL;
>      int ret = 0;

Preexisting, but I'll mention it anyway: This style of initialising ret
with 0 isn't wrong, but it would be more obviously correct if you set
ret = 0 only immediately before the out: label. The way it is currently,
I had to assure myself that write_object() really can't return a
positive number.

> -    bool result = true;
>      SheepdogInode *inode = &s->inode;
>  
> -    fd = connect_to_sdog(s, &local_err);
> +    fd = connect_to_sdog(s, errp);
>      if (fd < 0) {
> -        error_report_err(local_err);
> -        return false;
> +        return fd;
>      }
>  
>      nr_objs = count_data_objs(inode);
> @@ -2447,15 +2444,14 @@ static bool remove_objects(BDRVSheepdogState *s)
>                                      data_vdi_id[start_idx]),
>                             false, s->cache_flags);
>          if (ret < 0) {
> -            error_report("failed to discard snapshot inode.");
> -            result = false;
> +            error_setg(errp, "failed to discard snapshot inode.");

As Eric said, remove the trailing period. I would also let the message
start with a capital letter for consistency with the other error
messages in sd_snapshot_delete().

>              goto out;
>          }
>      }
>  
>  out:
>      closesocket(fd);
> -    return result;
> +    return ret;
>  }
>  
>  static int sd_snapshot_delete(BlockDriverState *bs,
> @@ -2465,7 +2461,6 @@ static int sd_snapshot_delete(BlockDriverState *bs,
>  {
>      unsigned long snap_id = 0;
>      char snap_tag[SD_MAX_VDI_TAG_LEN];
> -    Error *local_err = NULL;
>      int fd, ret;
>      char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN];
>      BDRVSheepdogState *s = bs->opaque;
> @@ -2478,8 +2473,9 @@ static int sd_snapshot_delete(BlockDriverState *bs,
>      };
>      SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr;
>  
> -    if (!remove_objects(s)) {
> -        return -1;
> +    ret = remove_objects(s, errp);
> +    if (ret) {
> +        return ret;
>      }
>  
>      memset(buf, 0, sizeof(buf));
> @@ -2500,35 +2496,36 @@ static int sd_snapshot_delete(BlockDriverState *bs,
>      }
>  
>      ret = find_vdi_name(s, s->name, snap_id, snap_tag, &vid, true,
> -                        &local_err);
> +                        errp);

This fits on a single line.

Nothing critical, but it seems you're going to send a new version
anyway, so you could as well improve it.

Kevin

Re: [Qemu-devel] [PATCH 02/15] sheepdog: Fix error handling in sd_snapshot_delete()

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 02.03.2017 um 22:43 hat Markus Armbruster geschrieben:
>> As a bdrv_snapshot_delete() method, sd_snapshot_delete() must set an
>> error and return negative errno on failure.  It sometimes returns -1,
>> and sometimes neglects to set an error.  It also prints error messages
>> with error_report().  Fix all that.
>> 
>> Moreover, its handling of an attempt to delete an nonexistent snapshot
>> is wrong: it error_report()s and succeeds.  Fix it to set an error and
>> return -ENOENT instead.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
>> -static bool remove_objects(BDRVSheepdogState *s)
>> +static int remove_objects(BDRVSheepdogState *s, Error **errp)
>>  {
>>      int fd, i = 0, nr_objs = 0;
>> -    Error *local_err = NULL;
>>      int ret = 0;
>
> Preexisting, but I'll mention it anyway: This style of initialising ret
> with 0 isn't wrong, but it would be more obviously correct if you set
> ret = 0 only immediately before the out: label. The way it is currently,
> I had to assure myself that write_object() really can't return a
> positive number.

I can squash in that change.

>> -    bool result = true;
>>      SheepdogInode *inode = &s->inode;
>>  
>> -    fd = connect_to_sdog(s, &local_err);
>> +    fd = connect_to_sdog(s, errp);
>>      if (fd < 0) {
>> -        error_report_err(local_err);
>> -        return false;
>> +        return fd;
>>      }
>>  
>>      nr_objs = count_data_objs(inode);
>> @@ -2447,15 +2444,14 @@ static bool remove_objects(BDRVSheepdogState *s)
>>                                      data_vdi_id[start_idx]),
>>                             false, s->cache_flags);
>>          if (ret < 0) {
>> -            error_report("failed to discard snapshot inode.");
>> -            result = false;
>> +            error_setg(errp, "failed to discard snapshot inode.");
>
> As Eric said, remove the trailing period. I would also let the message
> start with a capital letter for consistency with the other error
> messages in sd_snapshot_delete().

Okay.

Aside: we don't do this consistently either way.  For what it's worth,
lower case looks ugly when there is no prefix (e.g. in the human
monitor), and upper case can look ugly when there is one, particularly
with error_prepend().

>>              goto out;
>>          }
>>      }
>>  
>>  out:
>>      closesocket(fd);
>> -    return result;
>> +    return ret;
>>  }
>>  
>>  static int sd_snapshot_delete(BlockDriverState *bs,
>> @@ -2465,7 +2461,6 @@ static int sd_snapshot_delete(BlockDriverState *bs,
>>  {
>>      unsigned long snap_id = 0;
>>      char snap_tag[SD_MAX_VDI_TAG_LEN];
>> -    Error *local_err = NULL;
>>      int fd, ret;
>>      char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN];
>>      BDRVSheepdogState *s = bs->opaque;
>> @@ -2478,8 +2473,9 @@ static int sd_snapshot_delete(BlockDriverState *bs,
>>      };
>>      SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr;
>>  
>> -    if (!remove_objects(s)) {
>> -        return -1;
>> +    ret = remove_objects(s, errp);
>> +    if (ret) {
>> +        return ret;
>>      }
>>  
>>      memset(buf, 0, sizeof(buf));
>> @@ -2500,35 +2496,36 @@ static int sd_snapshot_delete(BlockDriverState *bs,
>>      }
>>  
>>      ret = find_vdi_name(s, s->name, snap_id, snap_tag, &vid, true,
>> -                        &local_err);
>> +                        errp);
>
> This fits on a single line.

Yes.

> Nothing critical, but it seems you're going to send a new version
> anyway, so you could as well improve it.

Sure!

[Qemu-devel] [PATCH 03/15] sheepdog: Fix error handling sd_create()

[Markus Armbruster]

As a bdrv_create() method, sd_create() must set an error and return
negative errno on failure.  It prints the error instead of setting it
when connect_to_sdog() fails.  Fix that.

While there, return the value of connect_to_sdog() like we do
elsewhere, instead of -EIO.  No functional change, as
connect_to_sdog() returns no other error code.

Many more suspicious uses of error_report() and error_report_err()
remain in other functions.  Left for another day.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/sheepdog.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index e4e5345..abfaa95 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1830,14 +1830,12 @@ static int sd_create(const char *filename, QemuOpts *opts,
     if (s->inode.block_size_shift == 0) {
         SheepdogVdiReq hdr;
         SheepdogClusterRsp *rsp = (SheepdogClusterRsp *)&hdr;
-        Error *local_err = NULL;
         int fd;
         unsigned int wlen = 0, rlen = 0;
 
-        fd = connect_to_sdog(s, &local_err);
+        fd = connect_to_sdog(s, errp);
         if (fd < 0) {
-            error_report_err(local_err);
-            ret = -EIO;
+            ret = fd;
             goto out;
         }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH 03/15] sheepdog: Fix error handling sd_create()

[Eric Blake]

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

On 03/02/2017 03:43 PM, Markus Armbruster wrote:
> As a bdrv_create() method, sd_create() must set an error and return
> negative errno on failure.  It prints the error instead of setting it
> when connect_to_sdog() fails.  Fix that.
> 
> While there, return the value of connect_to_sdog() like we do
> elsewhere, instead of -EIO.  No functional change, as
> connect_to_sdog() returns no other error code.
> 
> Many more suspicious uses of error_report() and error_report_err()
> remain in other functions.  Left for another day.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/sheepdog.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 

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

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


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

Re: [Qemu-devel] [PATCH 03/15] sheepdog: Fix error handling sd_create()

[Philippe Mathieu-Daudé]

On 03/02/2017 06:43 PM, Markus Armbruster wrote:
> As a bdrv_create() method, sd_create() must set an error and return
> negative errno on failure.  It prints the error instead of setting it
> when connect_to_sdog() fails.  Fix that.
>
> While there, return the value of connect_to_sdog() like we do
> elsewhere, instead of -EIO.  No functional change, as
> connect_to_sdog() returns no other error code.
>
> Many more suspicious uses of error_report() and error_report_err()
> remain in other functions.  Left for another day.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  block/sheepdog.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index e4e5345..abfaa95 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1830,14 +1830,12 @@ static int sd_create(const char *filename, QemuOpts *opts,
>      if (s->inode.block_size_shift == 0) {
>          SheepdogVdiReq hdr;
>          SheepdogClusterRsp *rsp = (SheepdogClusterRsp *)&hdr;
> -        Error *local_err = NULL;
>          int fd;
>          unsigned int wlen = 0, rlen = 0;
>
> -        fd = connect_to_sdog(s, &local_err);
> +        fd = connect_to_sdog(s, errp);
>          if (fd < 0) {
> -            error_report_err(local_err);
> -            ret = -EIO;
> +            ret = fd;
>              goto out;
>          }
>
>

Re: [Qemu-devel] [PATCH 03/15] sheepdog: Fix error handling sd_create()

[Kevin Wolf]

Am 02.03.2017 um 22:43 hat Markus Armbruster geschrieben:
> As a bdrv_create() method, sd_create() must set an error and return
> negative errno on failure.  It prints the error instead of setting it
> when connect_to_sdog() fails.  Fix that.
> 
> While there, return the value of connect_to_sdog() like we do
> elsewhere, instead of -EIO.  No functional change, as
> connect_to_sdog() returns no other error code.
> 
> Many more suspicious uses of error_report() and error_report_err()
> remain in other functions.  Left for another day.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 04/15] sheepdog: Mark sd_snapshot_delete() lossage FIXME

[Markus Armbruster]

sd_snapshot_delete() should delete the snapshot whose ID matches
@snapshot_id and whose name matches @name.  But that's not what it
does.  If @snapshot_id is a valid ID, it deletes the snapshot with
that ID, else it deletes the snapshot with that name.  It doesn't use
@name at all.  Add suitable FIXME comments, so someone who actually
knows Sheepdog can fix it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/sheepdog.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index abfaa95..5554f47 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2457,6 +2457,10 @@ static int sd_snapshot_delete(BlockDriverState *bs,
                               const char *name,
                               Error **errp)
 {
+    /*
+     * FIXME should delete the snapshot matching both @snapshot_id and
+     * @name, but @name not used here
+     */
     unsigned long snap_id = 0;
     char snap_tag[SD_MAX_VDI_TAG_LEN];
     int fd, ret;
@@ -2481,6 +2485,11 @@ static int sd_snapshot_delete(BlockDriverState *bs,
     pstrcpy(buf, SD_MAX_VDI_LEN, s->name);
     ret = qemu_strtoul(snapshot_id, NULL, 10, &snap_id);
     if (ret || snap_id > UINT32_MAX) {
+        /*
+         * FIXME Since qemu_strtoul() returns -EINVAL when
+         * @snapshot_id is null, @snapshot_id is mandatory.  Correct
+         * would be to require at least one of @snapshot_id and @name.
+         */
         error_setg(errp, "Invalid snapshot ID: %s",
                          snapshot_id ? snapshot_id : "<null>");
         return -EINVAL;
@@ -2489,6 +2498,7 @@ static int sd_snapshot_delete(BlockDriverState *bs,
     if (snap_id) {
         hdr.snapid = (uint32_t) snap_id;
     } else {
+        /* FIXME I suspect we should use @name here */
         pstrcpy(snap_tag, sizeof(snap_tag), snapshot_id);
         pstrcpy(buf + SD_MAX_VDI_LEN, SD_MAX_VDI_TAG_LEN, snap_tag);
     }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 04/15] sheepdog: Mark sd_snapshot_delete() lossage FIXME

[Eric Blake]

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

On 03/02/2017 03:43 PM, Markus Armbruster wrote:
> sd_snapshot_delete() should delete the snapshot whose ID matches
> @snapshot_id and whose name matches @name.  But that's not what it
> does.  If @snapshot_id is a valid ID, it deletes the snapshot with
> that ID, else it deletes the snapshot with that name.  It doesn't use
> @name at all.  Add suitable FIXME comments, so someone who actually
> knows Sheepdog can fix it.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/sheepdog.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 

If a sheepdog maintainer wants to rewrite this patch to avoid FIXME, I'm
fine with that. But in the meantime, this doesn't change behavior, so
it's safe for freeze.
Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

[Qemu-devel] [PATCH 05/15] sheepdog: Fix snapshot ID parsing in _open(), _create, _goto()

[Markus Armbruster]

sd_parse_uri() and sd_snapshot_goto() screw up error checking after
strtoul(), and truncate long tag names silently.  Fix by replacing
those parts by new sd_parse_snapid_or_tag(), which checks more
carefully.

sd_snapshot_delete() also parses snapshot IDs, but is currently too
broken for me to touch.  Mark TODO.

Two calls of strtol() without error checking remain in
parse_redundancy().  Mark them FIXME.

More silent truncation of configuration strings remains elsewhere.
Not marked.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/sheepdog.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 55 insertions(+), 11 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 5554f47..deb110e 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -914,6 +914,49 @@ static int get_sheep_fd(BDRVSheepdogState *s, Error **errp)
     return fd;
 }
 
+/*
+ * Parse numeric snapshot ID in @str
+ * If @str can't be parsed as number, return false.
+ * Else, if the number is zero or too large, set *@snapid to zero and
+ * return true.
+ * Else, set *@snapid to the number and return true.
+ */
+static bool sd_parse_snapid(const char *str, uint32_t *snapid)
+{
+    unsigned long ul;
+    int ret;
+
+    ret = qemu_strtoul(str, NULL, 10, &ul);
+    if (ret == -ERANGE) {
+        ul = ret = 0;
+    }
+    if (ret) {
+        return false;
+    }
+    if (ul > UINT32_MAX) {
+        ul = 0;
+    }
+
+    *snapid  = ul;
+    return true;
+}
+
+static bool sd_parse_snapid_or_tag(const char *str,
+                                   uint32_t *snapid, char tag[])
+{
+    if (!sd_parse_snapid(str, snapid)) {
+        *snapid = 0;
+        if (g_strlcpy(tag, str, SD_MAX_VDI_TAG_LEN) >= SD_MAX_VDI_TAG_LEN) {
+            return false;
+        }
+    } else if (!*snapid) {
+        return false;
+    } else {
+        tag[0] = 0;
+    }
+    return true;
+}
+
 static int sd_parse_uri(BDRVSheepdogState *s, const char *filename,
                         char *vdi, uint32_t *snapid, char *tag)
 {
@@ -965,9 +1008,9 @@ static int sd_parse_uri(BDRVSheepdogState *s, const char *filename,
 
     /* snapshot tag */
     if (uri->fragment) {
-        *snapid = strtoul(uri->fragment, NULL, 10);
-        if (*snapid == 0) {
-            pstrcpy(tag, SD_MAX_VDI_TAG_LEN, uri->fragment);
+        if (!sd_parse_snapid_or_tag(uri->fragment, snapid, tag)) {
+            ret = -EINVAL;
+            goto out;
         }
     } else {
         *snapid = CURRENT_VDI_ID; /* search current vdi */
@@ -1686,6 +1729,7 @@ static int parse_redundancy(BDRVSheepdogState *s, const char *opt)
     }
 
     copy = strtol(n1, NULL, 10);
+    /* FIXME fix error checking by switching to qemu_strtol() */
     if (copy > SD_MAX_COPIES || copy < 1) {
         return -EINVAL;
     }
@@ -1700,6 +1744,7 @@ static int parse_redundancy(BDRVSheepdogState *s, const char *opt)
     }
 
     parity = strtol(n2, NULL, 10);
+    /* FIXME fix error checking by switching to qemu_strtol() */
     if (parity >= SD_EC_MAX_STRIP || parity < 1) {
         return -EINVAL;
     }
@@ -2366,19 +2411,16 @@ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
     BDRVSheepdogState *old_s;
     char tag[SD_MAX_VDI_TAG_LEN];
     uint32_t snapid = 0;
-    int ret = 0;
+    int ret;
+
+    if (!sd_parse_snapid_or_tag(snapshot_id, &snapid, tag)) {
+        return -EINVAL;
+    }
 
     old_s = g_new(BDRVSheepdogState, 1);
 
     memcpy(old_s, s, sizeof(BDRVSheepdogState));
 
-    snapid = strtoul(snapshot_id, NULL, 10);
-    if (snapid) {
-        tag[0] = 0;
-    } else {
-        pstrcpy(tag, sizeof(tag), snapshot_id);
-    }
-
     ret = reload_inode(s, snapid, tag);
     if (ret) {
         goto out;
@@ -2483,6 +2525,7 @@ static int sd_snapshot_delete(BlockDriverState *bs,
     memset(buf, 0, sizeof(buf));
     memset(snap_tag, 0, sizeof(snap_tag));
     pstrcpy(buf, SD_MAX_VDI_LEN, s->name);
+    /* TODO Use sd_parse_snapid() once this mess is cleaned up */
     ret = qemu_strtoul(snapshot_id, NULL, 10, &snap_id);
     if (ret || snap_id > UINT32_MAX) {
         /*
@@ -2499,6 +2542,7 @@ static int sd_snapshot_delete(BlockDriverState *bs,
         hdr.snapid = (uint32_t) snap_id;
     } else {
         /* FIXME I suspect we should use @name here */
+        /* FIXME don't truncate silently */
         pstrcpy(snap_tag, sizeof(snap_tag), snapshot_id);
         pstrcpy(buf + SD_MAX_VDI_LEN, SD_MAX_VDI_TAG_LEN, snap_tag);
     }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 05/15] sheepdog: Fix snapshot ID parsing in _open(), _create, _goto()

[Eric Blake]

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

On 03/02/2017 03:43 PM, Markus Armbruster wrote:
> sd_parse_uri() and sd_snapshot_goto() screw up error checking after
> strtoul(), and truncate long tag names silently.  Fix by replacing
> those parts by new sd_parse_snapid_or_tag(), which checks more
> carefully.

At least we've fixed checkpatch to gripe at new uses of strtoul(), but
yeah, we've got lots of existing poor usage.  It is a very hard
interface to use correctly, as evidenced by your cleanups here.

> 
> sd_snapshot_delete() also parses snapshot IDs, but is currently too
> broken for me to touch.  Mark TODO.
> 
> Two calls of strtol() without error checking remain in
> parse_redundancy().  Mark them FIXME.
> 
> More silent truncation of configuration strings remains elsewhere.
> Not marked.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/sheepdog.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 55 insertions(+), 11 deletions(-)
> 

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

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


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

Re: [Qemu-devel] [PATCH 05/15] sheepdog: Fix snapshot ID parsing in _open(), _create, _goto()

[Kevin Wolf]

Am 02.03.2017 um 22:43 hat Markus Armbruster geschrieben:
> sd_parse_uri() and sd_snapshot_goto() screw up error checking after
> strtoul(), and truncate long tag names silently.  Fix by replacing
> those parts by new sd_parse_snapid_or_tag(), which checks more
> carefully.
> 
> sd_snapshot_delete() also parses snapshot IDs, but is currently too
> broken for me to touch.  Mark TODO.
> 
> Two calls of strtol() without error checking remain in
> parse_redundancy().  Mark them FIXME.
> 
> More silent truncation of configuration strings remains elsewhere.
> Not marked.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/sheepdog.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 55 insertions(+), 11 deletions(-)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 5554f47..deb110e 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -914,6 +914,49 @@ static int get_sheep_fd(BDRVSheepdogState *s, Error **errp)
>      return fd;
>  }
>  
> +/*
> + * Parse numeric snapshot ID in @str
> + * If @str can't be parsed as number, return false.
> + * Else, if the number is zero or too large, set *@snapid to zero and
> + * return true.
> + * Else, set *@snapid to the number and return true.
> + */
> +static bool sd_parse_snapid(const char *str, uint32_t *snapid)
> +{
> +    unsigned long ul;
> +    int ret;
> +
> +    ret = qemu_strtoul(str, NULL, 10, &ul);
> +    if (ret == -ERANGE) {
> +        ul = ret = 0;
> +    }
> +    if (ret) {
> +        return false;
> +    }
> +    if (ul > UINT32_MAX) {
> +        ul = 0;
> +    }
> +
> +    *snapid  = ul;

Redundant space.

> +    return true;
> +}

Looks good otherwise.

Kevin

Re: [Qemu-devel] [PATCH 05/15] sheepdog: Fix snapshot ID parsing in _open(), _create, _goto()

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 02.03.2017 um 22:43 hat Markus Armbruster geschrieben:
>> sd_parse_uri() and sd_snapshot_goto() screw up error checking after
>> strtoul(), and truncate long tag names silently.  Fix by replacing
>> those parts by new sd_parse_snapid_or_tag(), which checks more
>> carefully.
>> 
>> sd_snapshot_delete() also parses snapshot IDs, but is currently too
>> broken for me to touch.  Mark TODO.
>> 
>> Two calls of strtol() without error checking remain in
>> parse_redundancy().  Mark them FIXME.
>> 
>> More silent truncation of configuration strings remains elsewhere.
>> Not marked.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/sheepdog.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++----------
>>  1 file changed, 55 insertions(+), 11 deletions(-)
>> 
>> diff --git a/block/sheepdog.c b/block/sheepdog.c
>> index 5554f47..deb110e 100644
>> --- a/block/sheepdog.c
>> +++ b/block/sheepdog.c
>> @@ -914,6 +914,49 @@ static int get_sheep_fd(BDRVSheepdogState *s, Error **errp)
>>      return fd;
>>  }
>>  
>> +/*
>> + * Parse numeric snapshot ID in @str
>> + * If @str can't be parsed as number, return false.
>> + * Else, if the number is zero or too large, set *@snapid to zero and
>> + * return true.
>> + * Else, set *@snapid to the number and return true.
>> + */
>> +static bool sd_parse_snapid(const char *str, uint32_t *snapid)
>> +{
>> +    unsigned long ul;
>> +    int ret;
>> +
>> +    ret = qemu_strtoul(str, NULL, 10, &ul);
>> +    if (ret == -ERANGE) {
>> +        ul = ret = 0;
>> +    }
>> +    if (ret) {
>> +        return false;
>> +    }
>> +    if (ul > UINT32_MAX) {
>> +        ul = 0;
>> +    }
>> +
>> +    *snapid  = ul;
>
> Redundant space.

Will clean up.

>> +    return true;
>> +}
>
> Looks good otherwise.
>
> Kevin

Thanks!

[Qemu-devel] [PATCH 06/15] sheepdog: Don't truncate long VDI name in _open(), _create()

[Markus Armbruster]

sd_parse_uri() truncates long VDI names silently.  Reject them
instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/sheepdog.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index deb110e..72a52a6 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -985,7 +985,9 @@ static int sd_parse_uri(BDRVSheepdogState *s, const char *filename,
         ret = -EINVAL;
         goto out;
     }
-    pstrcpy(vdi, SD_MAX_VDI_LEN, uri->path + 1);
+    if (g_strlcpy(vdi, uri->path + 1, SD_MAX_VDI_LEN) >= SD_MAX_VDI_LEN) {
+        goto out;
+    }
 
     qp = query_params_parse(uri->query);
     if (qp->n > 1 || (s->is_unix && !qp->n) || (!s->is_unix && qp->n)) {
-- 
2.7.4

Re: [Qemu-devel] [PATCH 06/15] sheepdog: Don't truncate long VDI name in _open(), _create()

[Eric Blake]

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

On 03/02/2017 03:43 PM, Markus Armbruster wrote:
> sd_parse_uri() truncates long VDI names silently.  Reject them
> instead.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/sheepdog.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index deb110e..72a52a6 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -985,7 +985,9 @@ static int sd_parse_uri(BDRVSheepdogState *s, const char *filename,
>          ret = -EINVAL;
>          goto out;
>      }
> -    pstrcpy(vdi, SD_MAX_VDI_LEN, uri->path + 1);
> +    if (g_strlcpy(vdi, uri->path + 1, SD_MAX_VDI_LEN) >= SD_MAX_VDI_LEN) {
> +        goto out;
> +    }

Does this need to set ret? Maybe to -EINVAL?

>  
>      qp = query_params_parse(uri->query);
>      if (qp->n > 1 || (s->is_unix && !qp->n) || (!s->is_unix && qp->n)) {
> 

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


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

Re: [Qemu-devel] [PATCH 06/15] sheepdog: Don't truncate long VDI name in _open(), _create()

[Philippe Mathieu-Daudé]

On 03/02/2017 08:32 PM, Eric Blake wrote:
> On 03/02/2017 03:43 PM, Markus Armbruster wrote:
>> sd_parse_uri() truncates long VDI names silently.  Reject them
>> instead.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/sheepdog.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/sheepdog.c b/block/sheepdog.c
>> index deb110e..72a52a6 100644
>> --- a/block/sheepdog.c
>> +++ b/block/sheepdog.c
>> @@ -985,7 +985,9 @@ static int sd_parse_uri(BDRVSheepdogState *s, const char *filename,
>>          ret = -EINVAL;
>>          goto out;
>>      }
>> -    pstrcpy(vdi, SD_MAX_VDI_LEN, uri->path + 1);
>> +    if (g_strlcpy(vdi, uri->path + 1, SD_MAX_VDI_LEN) >= SD_MAX_VDI_LEN) {
>> +        goto out;
>> +    }
>
> Does this need to set ret? Maybe to -EINVAL?
>

ups I missed that. what about -ENAMETOOLONG?
bdrv callers seem to only test for 'ret < 0'.

>>
>>      qp = query_params_parse(uri->query);
>>      if (qp->n > 1 || (s->is_unix && !qp->n) || (!s->is_unix && qp->n)) {
>>
>

Re: [Qemu-devel] [PATCH 06/15] sheepdog: Don't truncate long VDI name in _open(), _create()

[Markus Armbruster]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> On 03/02/2017 08:32 PM, Eric Blake wrote:
>> On 03/02/2017 03:43 PM, Markus Armbruster wrote:
>>> sd_parse_uri() truncates long VDI names silently.  Reject them
>>> instead.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  block/sheepdog.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/block/sheepdog.c b/block/sheepdog.c
>>> index deb110e..72a52a6 100644
>>> --- a/block/sheepdog.c
>>> +++ b/block/sheepdog.c
>>> @@ -985,7 +985,9 @@ static int sd_parse_uri(BDRVSheepdogState *s, const char *filename,
>>>          ret = -EINVAL;
>>>          goto out;
>>>      }
>>> -    pstrcpy(vdi, SD_MAX_VDI_LEN, uri->path + 1);
>>> +    if (g_strlcpy(vdi, uri->path + 1, SD_MAX_VDI_LEN) >= SD_MAX_VDI_LEN) {
>>> +        goto out;
>>> +    }
>>
>> Does this need to set ret? Maybe to -EINVAL?
>>
>
> ups I missed that. what about -ENAMETOOLONG?
> bdrv callers seem to only test for 'ret < 0'.

The next patch gets rid of the error code in this function.

>>>
>>>      qp = query_params_parse(uri->query);
>>>      if (qp->n > 1 || (s->is_unix && !qp->n) || (!s->is_unix && qp->n)) {
>>>
>>

Re: [Qemu-devel] [PATCH 06/15] sheepdog: Don't truncate long VDI name in _open(), _create()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/02/2017 03:43 PM, Markus Armbruster wrote:
>> sd_parse_uri() truncates long VDI names silently.  Reject them
>> instead.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/sheepdog.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/block/sheepdog.c b/block/sheepdog.c
>> index deb110e..72a52a6 100644
>> --- a/block/sheepdog.c
>> +++ b/block/sheepdog.c
>> @@ -985,7 +985,9 @@ static int sd_parse_uri(BDRVSheepdogState *s, const char *filename,
>>          ret = -EINVAL;
>>          goto out;
>>      }
>> -    pstrcpy(vdi, SD_MAX_VDI_LEN, uri->path + 1);
>> +    if (g_strlcpy(vdi, uri->path + 1, SD_MAX_VDI_LEN) >= SD_MAX_VDI_LEN) {
>> +        goto out;
>> +    }
>
> Does this need to set ret? Maybe to -EINVAL?

Yes.  The next patch heals it, but of course I'll fix it anyway.

>>  
>>      qp = query_params_parse(uri->query);
>>      if (qp->n > 1 || (s->is_unix && !qp->n) || (!s->is_unix && qp->n)) {
>> 

Re: [Qemu-devel] [PATCH 06/15] sheepdog: Don't truncate long VDI name in _open(), _create()

[Philippe Mathieu-Daudé]

On 03/02/2017 06:43 PM, Markus Armbruster wrote:
> sd_parse_uri() truncates long VDI names silently.  Reject them
> instead.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  block/sheepdog.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index deb110e..72a52a6 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -985,7 +985,9 @@ static int sd_parse_uri(BDRVSheepdogState *s, const char *filename,
>          ret = -EINVAL;
>          goto out;
>      }
> -    pstrcpy(vdi, SD_MAX_VDI_LEN, uri->path + 1);
> +    if (g_strlcpy(vdi, uri->path + 1, SD_MAX_VDI_LEN) >= SD_MAX_VDI_LEN) {
> +        goto out;
> +    }
>
>      qp = query_params_parse(uri->query);
>      if (qp->n > 1 || (s->is_unix && !qp->n) || (!s->is_unix && qp->n)) {
>

[Qemu-devel] [PATCH 07/15] sheepdog: Report errors in pseudo-filename more usefully

[Markus Armbruster]

Errors in the pseudo-filename are all reported with the same laconic
"Can't parse filename" message.

Add real error reporting, such as:

    $ qemu-system-x86_64 --drive driver=sheepdog,filename=sheepdog:///
    qemu-system-x86_64: --drive driver=sheepdog,filename=sheepdog:///: missing file path in URI
    $ qemu-system-x86_64 --drive driver=sheepdog,filename=sheepgod:///vdi
    qemu-system-x86_64: --drive driver=sheepdog,filename=sheepgod:///vdi: URI scheme must be 'sheepdog', 'sheepdog+tcp', or 'sheepdog+unix'
    $ qemu-system-x86_64 --drive driver=sheepdog,filename=sheepdog+unix:///vdi?socke=sheepdog.sock
    qemu-system-x86_64: --drive driver=sheepdog,filename=sheepdog+unix:///vdi?socke=sheepdog.sock: unexpected query parameters

The code to translate legacy syntax to URI fails to escape URI
meta-characters.  The new error messages are misleading then.  Replace
them by the old "Can't parse filename" message.  "Internal error"
would be more honest.  Anyway, no worse than before.  Also add a FIXME
comment.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/sheepdog.c | 86 ++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 58 insertions(+), 28 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 72a52a6..ac62225 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -957,16 +957,18 @@ static bool sd_parse_snapid_or_tag(const char *str,
     return true;
 }
 
-static int sd_parse_uri(BDRVSheepdogState *s, const char *filename,
-                        char *vdi, uint32_t *snapid, char *tag)
+static void sd_parse_uri(BDRVSheepdogState *s, const char *filename,
+                         char *vdi, uint32_t *snapid, char *tag,
+                         Error **errp)
 {
+    Error *err = NULL;
     URI *uri;
     QueryParams *qp = NULL;
-    int ret = 0;
 
     uri = uri_parse(filename);
     if (!uri) {
-        return -EINVAL;
+        error_setg(&err, "invalid URI");
+        goto out;
     }
 
     /* transport */
@@ -977,33 +979,46 @@ static int sd_parse_uri(BDRVSheepdogState *s, const char *filename,
     } else if (!strcmp(uri->scheme, "sheepdog+unix")) {
         s->is_unix = true;
     } else {
-        ret = -EINVAL;
+        error_setg(&err, "URI scheme must be 'sheepdog', 'sheepdog+tcp',"
+                   " or 'sheepdog+unix'");
         goto out;
     }
 
     if (uri->path == NULL || !strcmp(uri->path, "/")) {
-        ret = -EINVAL;
+        error_setg(&err, "missing file path in URI");
         goto out;
     }
     if (g_strlcpy(vdi, uri->path + 1, SD_MAX_VDI_LEN) >= SD_MAX_VDI_LEN) {
+        error_setg(&err, "VDI name is too long");
         goto out;
     }
 
     qp = query_params_parse(uri->query);
-    if (qp->n > 1 || (s->is_unix && !qp->n) || (!s->is_unix && qp->n)) {
-        ret = -EINVAL;
-        goto out;
-    }
 
     if (s->is_unix) {
         /* sheepdog+unix:///vdiname?socket=path */
-        if (uri->server || uri->port || strcmp(qp->p[0].name, "socket")) {
-            ret = -EINVAL;
+        if (uri->server || uri->port) {
+            error_setg(&err, "URI scheme %s doesn't accept a server address",
+                       uri->scheme);
+            goto out;
+        }
+        if (!qp->n) {
+            error_setg(&err,
+                       "URI scheme %s requires query parameter 'socket'",
+                       uri->scheme);
+            goto out;
+        }
+        if (qp->n != 1 || strcmp(qp->p[0].name, "socket")) {
+            error_setg(&err, "unexpected query parameters");
             goto out;
         }
         s->host_spec = g_strdup(qp->p[0].value);
     } else {
         /* sheepdog[+tcp]://[host:port]/vdiname */
+        if (qp->n) {
+            error_setg(&err, "unexpected query parameters");
+            goto out;
+        }
         s->host_spec = g_strdup_printf("%s:%d", uri->server ?: SD_DEFAULT_ADDR,
                                        uri->port ?: SD_DEFAULT_PORT);
     }
@@ -1011,7 +1026,8 @@ static int sd_parse_uri(BDRVSheepdogState *s, const char *filename,
     /* snapshot tag */
     if (uri->fragment) {
         if (!sd_parse_snapid_or_tag(uri->fragment, snapid, tag)) {
-            ret = -EINVAL;
+            error_setg(&err, "'%s' is not a valid snapshot ID",
+                       uri->fragment);
             goto out;
         }
     } else {
@@ -1019,11 +1035,11 @@ static int sd_parse_uri(BDRVSheepdogState *s, const char *filename,
     }
 
 out:
+    error_propagate(errp, err);
     if (qp) {
         query_params_free(qp);
     }
     uri_free(uri);
-    return ret;
 }
 
 /*
@@ -1043,12 +1059,14 @@ out:
  * You can run VMs outside the Sheepdog cluster by specifying
  * `hostname' and `port' (experimental).
  */
-static int parse_vdiname(BDRVSheepdogState *s, const char *filename,
-                         char *vdi, uint32_t *snapid, char *tag)
+static void parse_vdiname(BDRVSheepdogState *s, const char *filename,
+                          char *vdi, uint32_t *snapid, char *tag,
+                          Error **errp)
 {
+    Error *err = NULL;
     char *p, *q, *uri;
     const char *host_spec, *vdi_spec;
-    int nr_sep, ret;
+    int nr_sep;
 
     strstart(filename, "sheepdog:", &filename);
     p = q = g_strdup(filename);
@@ -1083,12 +1101,23 @@ static int parse_vdiname(BDRVSheepdogState *s, const char *filename,
 
     uri = g_strdup_printf("sheepdog://%s/%s", host_spec, vdi_spec);
 
-    ret = sd_parse_uri(s, uri, vdi, snapid, tag);
+    /*
+     * FIXME We to escape URI meta-characters, e.g. "x?y=z"
+     * produces "sheepdog://x?y=z".  Because of that ...
+     */
+    sd_parse_uri(s, uri, vdi, snapid, tag, &err);
+    if (err) {
+        /*
+         * ... this can fail, but the error message is misleading.
+         * Replace it by the traditional useless one until the
+         * escaping is fixed.
+         */
+        error_free(err);
+        error_setg(errp, "Can't parse filename");
+    }
 
     g_free(q);
     g_free(uri);
-
-    return ret;
 }
 
 static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
@@ -1451,12 +1480,12 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
     memset(tag, 0, sizeof(tag));
 
     if (strstr(filename, "://")) {
-        ret = sd_parse_uri(s, filename, vdi, &snapid, tag);
+        sd_parse_uri(s, filename, vdi, &snapid, tag, &local_err);
     } else {
-        ret = parse_vdiname(s, filename, vdi, &snapid, tag);
+        parse_vdiname(s, filename, vdi, &snapid, tag, &local_err);
     }
-    if (ret < 0) {
-        error_setg(errp, "Can't parse filename");
+    if (local_err) {
+        error_propagate(errp, local_err);
         goto out_no_fd;
     }
     s->fd = get_sheep_fd(s, errp);
@@ -1785,6 +1814,7 @@ static int parse_block_size_shift(BDRVSheepdogState *s, QemuOpts *opt)
 static int sd_create(const char *filename, QemuOpts *opts,
                      Error **errp)
 {
+    Error *err = NULL;
     int ret = 0;
     uint32_t vid = 0;
     char *backing_file = NULL;
@@ -1799,12 +1829,12 @@ static int sd_create(const char *filename, QemuOpts *opts,
 
     memset(tag, 0, sizeof(tag));
     if (strstr(filename, "://")) {
-        ret = sd_parse_uri(s, filename, s->name, &snapid, tag);
+        sd_parse_uri(s, filename, s->name, &snapid, tag, &err);
     } else {
-        ret = parse_vdiname(s, filename, s->name, &snapid, tag);
+        parse_vdiname(s, filename, s->name, &snapid, tag, &err);
     }
-    if (ret < 0) {
-        error_setg(errp, "Can't parse filename");
+    if (err) {
+        error_propagate(errp, err);
         goto out;
     }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH 07/15] sheepdog: Report errors in pseudo-filename more usefully

[Kevin Wolf]

Am 02.03.2017 um 22:43 hat Markus Armbruster geschrieben:
> Errors in the pseudo-filename are all reported with the same laconic
> "Can't parse filename" message.
> 
> Add real error reporting, such as:
> 
>     $ qemu-system-x86_64 --drive driver=sheepdog,filename=sheepdog:///
>     qemu-system-x86_64: --drive driver=sheepdog,filename=sheepdog:///: missing file path in URI
>     $ qemu-system-x86_64 --drive driver=sheepdog,filename=sheepgod:///vdi
>     qemu-system-x86_64: --drive driver=sheepdog,filename=sheepgod:///vdi: URI scheme must be 'sheepdog', 'sheepdog+tcp', or 'sheepdog+unix'
>     $ qemu-system-x86_64 --drive driver=sheepdog,filename=sheepdog+unix:///vdi?socke=sheepdog.sock
>     qemu-system-x86_64: --drive driver=sheepdog,filename=sheepdog+unix:///vdi?socke=sheepdog.sock: unexpected query parameters
> 
> The code to translate legacy syntax to URI fails to escape URI
> meta-characters.  The new error messages are misleading then.  Replace
> them by the old "Can't parse filename" message.  "Internal error"
> would be more honest.  Anyway, no worse than before.  Also add a FIXME
> comment.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

More upper/lower case inconsistency in error messages. Maybe I should
simply ignore it.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH 07/15] sheepdog: Report errors in pseudo-filename more usefully

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 02.03.2017 um 22:43 hat Markus Armbruster geschrieben:
>> Errors in the pseudo-filename are all reported with the same laconic
>> "Can't parse filename" message.
>> 
>> Add real error reporting, such as:
>> 
>>     $ qemu-system-x86_64 --drive driver=sheepdog,filename=sheepdog:///
>>     qemu-system-x86_64: --drive driver=sheepdog,filename=sheepdog:///: missing file path in URI
>>     $ qemu-system-x86_64 --drive driver=sheepdog,filename=sheepgod:///vdi
>>     qemu-system-x86_64: --drive driver=sheepdog,filename=sheepgod:///vdi: URI scheme must be 'sheepdog', 'sheepdog+tcp', or 'sheepdog+unix'
>>     $ qemu-system-x86_64 --drive driver=sheepdog,filename=sheepdog+unix:///vdi?socke=sheepdog.sock
>>     qemu-system-x86_64: --drive driver=sheepdog,filename=sheepdog+unix:///vdi?socke=sheepdog.sock: unexpected query parameters
>> 
>> The code to translate legacy syntax to URI fails to escape URI
>> meta-characters.  The new error messages are misleading then.  Replace
>> them by the old "Can't parse filename" message.  "Internal error"
>> would be more honest.  Anyway, no worse than before.  Also add a FIXME
>> comment.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> More upper/lower case inconsistency in error messages. Maybe I should
> simply ignore it.

If you have a preference, I'm happy to use it for messages I touch.

> Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH 07/15] sheepdog: Report errors in pseudo-filename more usefully

[Kevin Wolf]

Am 02.03.2017 um 22:43 hat Markus Armbruster geschrieben:
> Errors in the pseudo-filename are all reported with the same laconic
> "Can't parse filename" message.
> 
> Add real error reporting, such as:
> 
>     $ qemu-system-x86_64 --drive driver=sheepdog,filename=sheepdog:///
>     qemu-system-x86_64: --drive driver=sheepdog,filename=sheepdog:///: missing file path in URI
>     $ qemu-system-x86_64 --drive driver=sheepdog,filename=sheepgod:///vdi
>     qemu-system-x86_64: --drive driver=sheepdog,filename=sheepgod:///vdi: URI scheme must be 'sheepdog', 'sheepdog+tcp', or 'sheepdog+unix'
>     $ qemu-system-x86_64 --drive driver=sheepdog,filename=sheepdog+unix:///vdi?socke=sheepdog.sock
>     qemu-system-x86_64: --drive driver=sheepdog,filename=sheepdog+unix:///vdi?socke=sheepdog.sock: unexpected query parameters
> 
> The code to translate legacy syntax to URI fails to escape URI
> meta-characters.  The new error messages are misleading then.  Replace
> them by the old "Can't parse filename" message.  "Internal error"
> would be more honest.  Anyway, no worse than before.  Also add a FIXME
> comment.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

> @@ -1451,12 +1480,12 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
>      memset(tag, 0, sizeof(tag));
>  
>      if (strstr(filename, "://")) {
> -        ret = sd_parse_uri(s, filename, vdi, &snapid, tag);
> +        sd_parse_uri(s, filename, vdi, &snapid, tag, &local_err);
>      } else {
> -        ret = parse_vdiname(s, filename, vdi, &snapid, tag);
> +        parse_vdiname(s, filename, vdi, &snapid, tag, &local_err);
>      }
> -    if (ret < 0) {
> -        error_setg(errp, "Can't parse filename");
> +    if (local_err) {
> +        error_propagate(errp, local_err);
>          goto out_no_fd;
>      }

I have to take my R-b back, ret isn't set here any more:

block/sheepdog.c: In function 'sd_open':
block/sheepdog.c:1451:9: error: 'ret' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     int ret, fd;

Kevin

Re: [Qemu-devel] [PATCH 07/15] sheepdog: Report errors in pseudo-filename more usefully

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 02.03.2017 um 22:43 hat Markus Armbruster geschrieben:
>> Errors in the pseudo-filename are all reported with the same laconic
>> "Can't parse filename" message.
>> 
>> Add real error reporting, such as:
>> 
>>     $ qemu-system-x86_64 --drive driver=sheepdog,filename=sheepdog:///
>>     qemu-system-x86_64: --drive driver=sheepdog,filename=sheepdog:///: missing file path in URI
>>     $ qemu-system-x86_64 --drive driver=sheepdog,filename=sheepgod:///vdi
>>     qemu-system-x86_64: --drive driver=sheepdog,filename=sheepgod:///vdi: URI scheme must be 'sheepdog', 'sheepdog+tcp', or 'sheepdog+unix'
>>     $ qemu-system-x86_64 --drive driver=sheepdog,filename=sheepdog+unix:///vdi?socke=sheepdog.sock
>>     qemu-system-x86_64: --drive driver=sheepdog,filename=sheepdog+unix:///vdi?socke=sheepdog.sock: unexpected query parameters
>> 
>> The code to translate legacy syntax to URI fails to escape URI
>> meta-characters.  The new error messages are misleading then.  Replace
>> them by the old "Can't parse filename" message.  "Internal error"
>> would be more honest.  Anyway, no worse than before.  Also add a FIXME
>> comment.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
>> @@ -1451,12 +1480,12 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
>>      memset(tag, 0, sizeof(tag));
>>  
>>      if (strstr(filename, "://")) {
>> -        ret = sd_parse_uri(s, filename, vdi, &snapid, tag);
>> +        sd_parse_uri(s, filename, vdi, &snapid, tag, &local_err);
>>      } else {
>> -        ret = parse_vdiname(s, filename, vdi, &snapid, tag);
>> +        parse_vdiname(s, filename, vdi, &snapid, tag, &local_err);
>>      }
>> -    if (ret < 0) {
>> -        error_setg(errp, "Can't parse filename");
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>>          goto out_no_fd;
>>      }
>
> I have to take my R-b back, ret isn't set here any more:
>
> block/sheepdog.c: In function 'sd_open':
> block/sheepdog.c:1451:9: error: 'ret' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>      int ret, fd;

Healed in PATCH 09, but of course I'll fix it anyway.

[Qemu-devel] [PATCH 08/15] sheepdog: Use SocketAddress and socket_connect()

[Markus Armbruster]

sd_parse_uri() builds a string from host and port parts for
inet_connect().  inet_connect() parses it into host, port and options.
Whether this gets exactly the same host, port and no options for all
inputs is not obvious.

Cut out the string middleman and build a SocketAddress for
socket_connect() instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/sheepdog.c | 53 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 30 insertions(+), 23 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index ac62225..43317b3 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -374,8 +374,7 @@ struct BDRVSheepdogState {
     uint32_t cache_flags;
     bool discard_supported;
 
-    char *host_spec;
-    bool is_unix;
+    SocketAddress *addr;
     int fd;
 
     CoMutex lock;
@@ -532,16 +531,12 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
 {
     int fd;
 
-    if (s->is_unix) {
-        fd = unix_connect(s->host_spec, errp);
-    } else {
-        fd = inet_connect(s->host_spec, errp);
+    fd = socket_connect(s->addr, errp, NULL, NULL);
 
-        if (fd >= 0) {
-            int ret = socket_set_nodelay(fd);
-            if (ret < 0) {
-                error_report("%s", strerror(errno));
-            }
+    if (s->addr->type == SOCKET_ADDRESS_KIND_INET && fd >= 0) {
+        int ret = socket_set_nodelay(fd);
+        if (ret < 0) {
+            error_report("%s", strerror(errno));
         }
     }
 
@@ -820,8 +815,7 @@ static void coroutine_fn aio_read_response(void *opaque)
     case AIOCB_DISCARD_OBJ:
         switch (rsp.result) {
         case SD_RES_INVALID_PARMS:
-            error_report("sheep(%s) doesn't support discard command",
-                         s->host_spec);
+            error_report("server doesn't support discard command");
             rsp.result = SD_RES_SUCCESS;
             s->discard_supported = false;
             break;
@@ -962,8 +956,10 @@ static void sd_parse_uri(BDRVSheepdogState *s, const char *filename,
                          Error **errp)
 {
     Error *err = NULL;
-    URI *uri;
     QueryParams *qp = NULL;
+    SocketAddress *addr = NULL;
+    bool is_unix;
+    URI *uri;
 
     uri = uri_parse(filename);
     if (!uri) {
@@ -973,11 +969,11 @@ static void sd_parse_uri(BDRVSheepdogState *s, const char *filename,
 
     /* transport */
     if (!strcmp(uri->scheme, "sheepdog")) {
-        s->is_unix = false;
+        is_unix = false;
     } else if (!strcmp(uri->scheme, "sheepdog+tcp")) {
-        s->is_unix = false;
+        is_unix = false;
     } else if (!strcmp(uri->scheme, "sheepdog+unix")) {
-        s->is_unix = true;
+        is_unix = true;
     } else {
         error_setg(&err, "URI scheme must be 'sheepdog', 'sheepdog+tcp',"
                    " or 'sheepdog+unix'");
@@ -994,8 +990,9 @@ static void sd_parse_uri(BDRVSheepdogState *s, const char *filename,
     }
 
     qp = query_params_parse(uri->query);
+    addr = g_new0(SocketAddress, 1);
 
-    if (s->is_unix) {
+    if (is_unix) {
         /* sheepdog+unix:///vdiname?socket=path */
         if (uri->server || uri->port) {
             error_setg(&err, "URI scheme %s doesn't accept a server address",
@@ -1012,15 +1009,20 @@ static void sd_parse_uri(BDRVSheepdogState *s, const char *filename,
             error_setg(&err, "unexpected query parameters");
             goto out;
         }
-        s->host_spec = g_strdup(qp->p[0].value);
+        addr->type = SOCKET_ADDRESS_KIND_UNIX;
+        addr->u.q_unix.data = g_new0(UnixSocketAddress, 1);
+        addr->u.q_unix.data->path = g_strdup(qp->p[0].value);
     } else {
         /* sheepdog[+tcp]://[host:port]/vdiname */
         if (qp->n) {
             error_setg(&err, "unexpected query parameters");
             goto out;
         }
-        s->host_spec = g_strdup_printf("%s:%d", uri->server ?: SD_DEFAULT_ADDR,
-                                       uri->port ?: SD_DEFAULT_PORT);
+        addr->type = SOCKET_ADDRESS_KIND_INET;
+        addr->u.inet.data = g_new0(InetSocketAddress, 1);
+        addr->u.inet.data->host = g_strdup(uri->server ?: SD_DEFAULT_ADDR);
+        addr->u.inet.data->port = g_strdup_printf("%d",
+                                        uri->port ?: SD_DEFAULT_PORT);
     }
 
     /* snapshot tag */
@@ -1035,7 +1037,12 @@ static void sd_parse_uri(BDRVSheepdogState *s, const char *filename,
     }
 
 out:
-    error_propagate(errp, err);
+    if (err) {
+        error_propagate(errp, err);
+        qapi_free_SocketAddress(addr);
+    } else {
+        s->addr = addr;
+    }
     if (qp) {
         query_params_free(qp);
     }
@@ -1998,7 +2005,7 @@ static void sd_close(BlockDriverState *bs)
     aio_set_fd_handler(bdrv_get_aio_context(bs), s->fd,
                        false, NULL, NULL, NULL, NULL);
     closesocket(s->fd);
-    g_free(s->host_spec);
+    qapi_free_SocketAddress(s->addr);
 }
 
 static int64_t sd_getlength(BlockDriverState *bs)
-- 
2.7.4

Re: [Qemu-devel] [PATCH 08/15] sheepdog: Use SocketAddress and socket_connect()

[Kevin Wolf]

Am 02.03.2017 um 22:43 hat Markus Armbruster geschrieben:
> sd_parse_uri() builds a string from host and port parts for
> inet_connect().  inet_connect() parses it into host, port and options.
> Whether this gets exactly the same host, port and no options for all
> inputs is not obvious.
> 
> Cut out the string middleman and build a SocketAddress for
> socket_connect() instead.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 09/15] sheepdog: Implement bdrv_parse_filename()

[Markus Armbruster]

This permits configuration with driver-specific options in addition to
pseudo-filename parsed as URI.  For instance,

    --drive driver=sheepdog,host=fido,vdi=dolly

instead of

    --drive driver=sheepdog,file=sheepdog://fido/dolly

It's also a first step towards supporting blockdev-add.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/sheepdog.c | 233 +++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 176 insertions(+), 57 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 43317b3..087c4d6 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -14,6 +14,8 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qint.h"
 #include "qemu/uri.h"
 #include "qemu/error-report.h"
 #include "qemu/sockets.h"
@@ -526,6 +528,25 @@ static void sd_aio_setup(SheepdogAIOCB *acb, BDRVSheepdogState *s,
     QLIST_INSERT_HEAD(&s->inflight_aiocb_head, acb, aiocb_siblings);
 }
 
+static SocketAddress *sd_socket_address(const char *path,
+                                        const char *host, const char *port)
+{
+    SocketAddress *addr = g_new0(SocketAddress, 1);
+
+    if (path) {
+        addr->type = SOCKET_ADDRESS_KIND_UNIX;
+        addr->u.q_unix.data = g_new0(UnixSocketAddress, 1);
+        addr->u.q_unix.data->path = g_strdup(path);
+    } else {
+        addr->type = SOCKET_ADDRESS_KIND_INET;
+        addr->u.inet.data = g_new0(InetSocketAddress, 1);
+        addr->u.inet.data->host = g_strdup(host ?: SD_DEFAULT_ADDR);
+        addr->u.inet.data->port = g_strdup(port ?: stringify(SD_DEFAULT_PORT));
+    }
+
+    return addr;
+}
+
 /* Return -EIO in case of error, file descriptor on success */
 static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
 {
@@ -951,17 +972,37 @@ static bool sd_parse_snapid_or_tag(const char *str,
     return true;
 }
 
-static void sd_parse_uri(BDRVSheepdogState *s, const char *filename,
-                         char *vdi, uint32_t *snapid, char *tag,
+typedef struct {
+    const char *path;           /* non-null iff transport is tcp */
+    const char *host;           /* valid when transport is tcp */
+    int port;                   /* valid when transport is tcp */
+    char vdi[SD_MAX_VDI_LEN];
+    char tag[SD_MAX_VDI_TAG_LEN];
+    uint32_t snap_id;
+    /* Remainder is only for sd_config_done() */
+    URI *uri;
+    QueryParams *qp;
+} SheepdogConfig;
+
+static void sd_config_done(SheepdogConfig *cfg)
+{
+    if (cfg->qp) {
+        query_params_free(cfg->qp);
+    }
+    uri_free(cfg->uri);
+}
+
+static void sd_parse_uri(SheepdogConfig *cfg, const char *filename,
                          Error **errp)
 {
     Error *err = NULL;
     QueryParams *qp = NULL;
-    SocketAddress *addr = NULL;
     bool is_unix;
     URI *uri;
 
-    uri = uri_parse(filename);
+    memset(cfg, 0, sizeof(*cfg));
+
+    cfg->uri = uri = uri_parse(filename);
     if (!uri) {
         error_setg(&err, "invalid URI");
         goto out;
@@ -984,13 +1025,13 @@ static void sd_parse_uri(BDRVSheepdogState *s, const char *filename,
         error_setg(&err, "missing file path in URI");
         goto out;
     }
-    if (g_strlcpy(vdi, uri->path + 1, SD_MAX_VDI_LEN) >= SD_MAX_VDI_LEN) {
+    if (g_strlcpy(cfg->vdi, uri->path + 1, SD_MAX_VDI_LEN)
+        >= SD_MAX_VDI_LEN) {
         error_setg(&err, "VDI name is too long");
         goto out;
     }
 
-    qp = query_params_parse(uri->query);
-    addr = g_new0(SocketAddress, 1);
+    cfg->qp = qp = query_params_parse(uri->query);
 
     if (is_unix) {
         /* sheepdog+unix:///vdiname?socket=path */
@@ -1009,44 +1050,34 @@ static void sd_parse_uri(BDRVSheepdogState *s, const char *filename,
             error_setg(&err, "unexpected query parameters");
             goto out;
         }
-        addr->type = SOCKET_ADDRESS_KIND_UNIX;
-        addr->u.q_unix.data = g_new0(UnixSocketAddress, 1);
-        addr->u.q_unix.data->path = g_strdup(qp->p[0].value);
+        cfg->path = qp->p[0].value;
     } else {
         /* sheepdog[+tcp]://[host:port]/vdiname */
         if (qp->n) {
             error_setg(&err, "unexpected query parameters");
             goto out;
         }
-        addr->type = SOCKET_ADDRESS_KIND_INET;
-        addr->u.inet.data = g_new0(InetSocketAddress, 1);
-        addr->u.inet.data->host = g_strdup(uri->server ?: SD_DEFAULT_ADDR);
-        addr->u.inet.data->port = g_strdup_printf("%d",
-                                        uri->port ?: SD_DEFAULT_PORT);
+        cfg->host = uri->server;
+        cfg->port = uri->port;
     }
 
     /* snapshot tag */
     if (uri->fragment) {
-        if (!sd_parse_snapid_or_tag(uri->fragment, snapid, tag)) {
+        if (!sd_parse_snapid_or_tag(uri->fragment,
+                                    &cfg->snap_id, cfg->tag)) {
             error_setg(&err, "'%s' is not a valid snapshot ID",
                        uri->fragment);
             goto out;
         }
     } else {
-        *snapid = CURRENT_VDI_ID; /* search current vdi */
+        cfg->snap_id = CURRENT_VDI_ID; /* search current vdi */
     }
 
 out:
     if (err) {
         error_propagate(errp, err);
-        qapi_free_SocketAddress(addr);
-    } else {
-        s->addr = addr;
+        sd_config_done(cfg);
     }
-    if (qp) {
-        query_params_free(qp);
-    }
-    uri_free(uri);
 }
 
 /*
@@ -1066,8 +1097,7 @@ out:
  * You can run VMs outside the Sheepdog cluster by specifying
  * `hostname' and `port' (experimental).
  */
-static void parse_vdiname(BDRVSheepdogState *s, const char *filename,
-                          char *vdi, uint32_t *snapid, char *tag,
+static void parse_vdiname(SheepdogConfig *cfg, const char *filename,
                           Error **errp)
 {
     Error *err = NULL;
@@ -1112,7 +1142,7 @@ static void parse_vdiname(BDRVSheepdogState *s, const char *filename,
      * FIXME We to escape URI meta-characters, e.g. "x?y=z"
      * produces "sheepdog://x?y=z".  Because of that ...
      */
-    sd_parse_uri(s, uri, vdi, snapid, tag, &err);
+    sd_parse_uri(cfg, uri, &err);
     if (err) {
         /*
          * ... this can fail, but the error message is misleading.
@@ -1127,6 +1157,43 @@ static void parse_vdiname(BDRVSheepdogState *s, const char *filename,
     g_free(uri);
 }
 
+static void sd_parse_filename(const char *filename, QDict *options,
+                              Error **errp)
+{
+    Error *err = NULL;
+    SheepdogConfig cfg;
+    char buf[32];
+
+    if (strstr(filename, "://")) {
+        sd_parse_uri(&cfg, filename, &err);
+    } else {
+        parse_vdiname(&cfg, filename, &err);
+    }
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
+    if (cfg.host) {
+        qdict_set_default_str(options, "host", cfg.host);
+    }
+    if (cfg.port) {
+        snprintf(buf, sizeof(buf), "%d", cfg.port);
+        qdict_set_default_str(options, "port", buf);
+    }
+    if (cfg.path) {
+        qdict_set_default_str(options, "path", cfg.path);
+    }
+    qdict_set_default_str(options, "vdi", cfg.vdi);
+    qdict_set_default_str(options, "tag", cfg.tag);
+    if (cfg.snap_id) {
+        snprintf(buf, sizeof(buf), "%d", cfg.snap_id);
+        qdict_set_default_str(options, "snap-id", buf);
+    }
+
+    sd_config_done(&cfg);
+}
+
 static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
                          uint32_t snapid, const char *tag, uint32_t *vid,
                          bool lock, Error **errp)
@@ -1438,15 +1505,33 @@ static void sd_attach_aio_context(BlockDriverState *bs,
                        co_read_response, NULL, NULL, s);
 }
 
-/* TODO Convert to fine grained options */
 static QemuOptsList runtime_opts = {
     .name = "sheepdog",
     .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
     .desc = {
         {
-            .name = "filename",
+            .name = "host",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            .name = "port",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            .name = "path",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            .name = "vdi",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            .name = "snap-id",
+            .type = QEMU_OPT_NUMBER,
+        },
+        {
+            .name = "tag",
             .type = QEMU_OPT_STRING,
-            .help = "URL to the sheepdog image",
         },
         { /* end of list */ }
     },
@@ -1458,12 +1543,11 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
     int ret, fd;
     uint32_t vid = 0;
     BDRVSheepdogState *s = bs->opaque;
-    char vdi[SD_MAX_VDI_LEN], tag[SD_MAX_VDI_TAG_LEN];
-    uint32_t snapid;
+    const char *host, *port, *path, *vdi, *snap_id_str, *tag;
+    uint64_t snap_id;
     char *buf = NULL;
     QemuOpts *opts;
     Error *local_err = NULL;
-    const char *filename;
 
     s->bs = bs;
     s->aio_context = bdrv_get_aio_context(bs);
@@ -1476,32 +1560,63 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
         goto out_no_fd;
     }
 
-    filename = qemu_opt_get(opts, "filename");
+    host = qemu_opt_get(opts, "host");
+    port = qemu_opt_get(opts, "port");
+    path = qemu_opt_get(opts, "path");
+    vdi = qemu_opt_get(opts, "vdi");
+    snap_id_str = qemu_opt_get(opts, "snap-id");
+    snap_id = qemu_opt_get_number(opts, "snap-id", CURRENT_VDI_ID);
+    tag = qemu_opt_get(opts, "tag");
+
+    if ((host || port) && path) {
+        error_setg(errp, "can't use 'path' together with 'host' or 'port'");
+        ret = -EINVAL;
+        goto out_no_fd;
+    }
+
+    if (!vdi) {
+        error_setg(errp, "parameter 'vdi' is missing");
+        ret = -EINVAL;
+        goto out_no_fd;
+    }
+    if (strlen(vdi) >= SD_MAX_VDI_LEN) {
+        error_setg(errp, "value of parameter 'vdi' is too long");
+        ret = -EINVAL;
+        goto out_no_fd;
+    }
+
+    if (snap_id > UINT32_MAX) {
+        snap_id = 0;
+    }
+    if (snap_id_str && !snap_id) {
+        error_setg(errp, "'snap-id=%s' is not a valid snapshot ID",
+                   snap_id_str);
+        ret = -EINVAL;
+        goto out_no_fd;
+    }
+
+    if (!tag) {
+        tag = "";
+    }
+    if (tag && strlen(tag) >= SD_MAX_VDI_TAG_LEN) {
+        error_setg(errp, "value of parameter 'tag' is too long");
+        ret = -EINVAL;
+        goto out_no_fd;
+    }
+
+    s->addr = sd_socket_address(path, host, port);
 
     QLIST_INIT(&s->inflight_aio_head);
     QLIST_INIT(&s->failed_aio_head);
     QLIST_INIT(&s->inflight_aiocb_head);
-    s->fd = -1;
 
-    memset(vdi, 0, sizeof(vdi));
-    memset(tag, 0, sizeof(tag));
-
-    if (strstr(filename, "://")) {
-        sd_parse_uri(s, filename, vdi, &snapid, tag, &local_err);
-    } else {
-        parse_vdiname(s, filename, vdi, &snapid, tag, &local_err);
-    }
-    if (local_err) {
-        error_propagate(errp, local_err);
-        goto out_no_fd;
-    }
     s->fd = get_sheep_fd(s, errp);
     if (s->fd < 0) {
         ret = s->fd;
         goto out_no_fd;
     }
 
-    ret = find_vdi_name(s, vdi, snapid, tag, &vid, true, errp);
+    ret = find_vdi_name(s, vdi, (uint32_t)snap_id, tag, &vid, true, errp);
     if (ret) {
         goto out;
     }
@@ -1516,7 +1631,7 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
     }
     s->discard_supported = true;
 
-    if (snapid || tag[0] != '\0') {
+    if (snap_id || tag[0]) {
         DPRINTF("%" PRIx32 " snapshot inode was open.\n", vid);
         s->is_snapshot = true;
     }
@@ -1827,24 +1942,28 @@ static int sd_create(const char *filename, QemuOpts *opts,
     char *backing_file = NULL;
     char *buf = NULL;
     BDRVSheepdogState *s;
-    char tag[SD_MAX_VDI_TAG_LEN];
-    uint32_t snapid;
+    SheepdogConfig cfg;
     uint64_t max_vdi_size;
     bool prealloc = false;
 
     s = g_new0(BDRVSheepdogState, 1);
 
-    memset(tag, 0, sizeof(tag));
     if (strstr(filename, "://")) {
-        sd_parse_uri(s, filename, s->name, &snapid, tag, &err);
+        sd_parse_uri(&cfg, filename, &err);
     } else {
-        parse_vdiname(s, filename, s->name, &snapid, tag, &err);
+        parse_vdiname(&cfg, filename, &err);
     }
     if (err) {
         error_propagate(errp, err);
         goto out;
     }
 
+    buf = cfg.port ? g_strdup_printf("%d", cfg.port) : NULL;
+    s->addr = sd_socket_address(cfg.path, cfg.host, buf);
+    g_free(buf);
+    strcpy(s->name, cfg.vdi);
+    sd_config_done(&cfg);
+
     s->inode.vdi_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
                                  BDRV_SECTOR_SIZE);
     backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
@@ -2921,7 +3040,7 @@ static BlockDriver bdrv_sheepdog = {
     .format_name    = "sheepdog",
     .protocol_name  = "sheepdog",
     .instance_size  = sizeof(BDRVSheepdogState),
-    .bdrv_needs_filename = true,
+    .bdrv_parse_filename    = sd_parse_filename,
     .bdrv_file_open = sd_open,
     .bdrv_reopen_prepare    = sd_reopen_prepare,
     .bdrv_reopen_commit     = sd_reopen_commit,
@@ -2957,7 +3076,7 @@ static BlockDriver bdrv_sheepdog_tcp = {
     .format_name    = "sheepdog",
     .protocol_name  = "sheepdog+tcp",
     .instance_size  = sizeof(BDRVSheepdogState),
-    .bdrv_needs_filename = true,
+    .bdrv_parse_filename    = sd_parse_filename,
     .bdrv_file_open = sd_open,
     .bdrv_reopen_prepare    = sd_reopen_prepare,
     .bdrv_reopen_commit     = sd_reopen_commit,
@@ -2993,7 +3112,7 @@ static BlockDriver bdrv_sheepdog_unix = {
     .format_name    = "sheepdog",
     .protocol_name  = "sheepdog+unix",
     .instance_size  = sizeof(BDRVSheepdogState),
-    .bdrv_needs_filename = true,
+    .bdrv_parse_filename    = sd_parse_filename,
     .bdrv_file_open = sd_open,
     .bdrv_reopen_prepare    = sd_reopen_prepare,
     .bdrv_reopen_commit     = sd_reopen_commit,
-- 
2.7.4

Re: [Qemu-devel] [PATCH 09/15] sheepdog: Implement bdrv_parse_filename()

[Eric Blake]

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

On 03/02/2017 03:44 PM, Markus Armbruster wrote:
> This permits configuration with driver-specific options in addition to
> pseudo-filename parsed as URI.  For instance,
> 
>     --drive driver=sheepdog,host=fido,vdi=dolly
> 
> instead of
> 
>     --drive driver=sheepdog,file=sheepdog://fido/dolly
> 
> It's also a first step towards supporting blockdev-add.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/sheepdog.c | 233 +++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 176 insertions(+), 57 deletions(-)
> 

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

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


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

[Qemu-devel] [PATCH 10/15] gluster: Drop assumptions on SocketTransport names

[Markus Armbruster]

qemu_gluster_glfs_init() passes the names of QAPI enumeration type
SocketTransport to glfs_set_volfile_server().  Works, because they
were chosen to match.  But the coupling is artificial.  Use the
appropriate literal strings instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/gluster.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 56b4abe..7236d59 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -412,8 +412,7 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
 
     for (server = gconf->server; server; server = server->next) {
         if (server->value->type  == GLUSTER_TRANSPORT_UNIX) {
-            ret = glfs_set_volfile_server(glfs,
-                                   GlusterTransport_lookup[server->value->type],
+            ret = glfs_set_volfile_server(glfs, "unix",
                                    server->value->u.q_unix.path, 0);
         } else {
             if (parse_uint_full(server->value->u.tcp.port, &port, 10) < 0 ||
@@ -423,8 +422,7 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
                 errno = EINVAL;
                 goto out;
             }
-            ret = glfs_set_volfile_server(glfs,
-                                   GlusterTransport_lookup[server->value->type],
+            ret = glfs_set_volfile_server(glfs, "tcp",
                                    server->value->u.tcp.host,
                                    (int)port);
         }
-- 
2.7.4

Re: [Qemu-devel] [Qemu-block] [PATCH 10/15] gluster: Drop assumptions on SocketTransport names

[Niels de Vos]

On Thu, Mar 02, 2017 at 10:44:01PM +0100, Markus Armbruster wrote:
> qemu_gluster_glfs_init() passes the names of QAPI enumeration type
> SocketTransport to glfs_set_volfile_server().  Works, because they
> were chosen to match.  But the coupling is artificial.  Use the
> appropriate literal strings instead.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/gluster.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/block/gluster.c b/block/gluster.c
> index 56b4abe..7236d59 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -412,8 +412,7 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
>  
>      for (server = gconf->server; server; server = server->next) {
>          if (server->value->type  == GLUSTER_TRANSPORT_UNIX) {
> -            ret = glfs_set_volfile_server(glfs,
> -                                   GlusterTransport_lookup[server->value->type],
> +            ret = glfs_set_volfile_server(glfs, "unix",
>                                     server->value->u.q_unix.path, 0);
>          } else {
>              if (parse_uint_full(server->value->u.tcp.port, &port, 10) < 0 ||
> @@ -423,8 +422,7 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
>                  errno = EINVAL;
>                  goto out;
>              }
> -            ret = glfs_set_volfile_server(glfs,
> -                                   GlusterTransport_lookup[server->value->type],
> +            ret = glfs_set_volfile_server(glfs, "tcp",
>                                     server->value->u.tcp.host,
>                                     (int)port);
>          }
> -- 
> 2.7.4

Instead of the strings for "unix" and "tcp", I would have liked
#define's. Unfortunately it seems that these are not available in public
headers :-/

If this is easier to understand, I don't have any objections.

Reviewed-by: Niels de Vos <ndevos@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH 10/15] gluster: Drop assumptions on SocketTransport names

[Markus Armbruster]

Niels de Vos <ndevos@redhat.com> writes:

> On Thu, Mar 02, 2017 at 10:44:01PM +0100, Markus Armbruster wrote:
>> qemu_gluster_glfs_init() passes the names of QAPI enumeration type
>> SocketTransport to glfs_set_volfile_server().  Works, because they
>> were chosen to match.  But the coupling is artificial.  Use the
>> appropriate literal strings instead.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/gluster.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>> 
>> diff --git a/block/gluster.c b/block/gluster.c
>> index 56b4abe..7236d59 100644
>> --- a/block/gluster.c
>> +++ b/block/gluster.c
>> @@ -412,8 +412,7 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
>>  
>>      for (server = gconf->server; server; server = server->next) {
>>          if (server->value->type  == GLUSTER_TRANSPORT_UNIX) {
>> -            ret = glfs_set_volfile_server(glfs,
>> -                                   GlusterTransport_lookup[server->value->type],
>> +            ret = glfs_set_volfile_server(glfs, "unix",
>>                                     server->value->u.q_unix.path, 0);
>>          } else {
>>              if (parse_uint_full(server->value->u.tcp.port, &port, 10) < 0 ||
>> @@ -423,8 +422,7 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
>>                  errno = EINVAL;
>>                  goto out;
>>              }
>> -            ret = glfs_set_volfile_server(glfs,
>> -                                   GlusterTransport_lookup[server->value->type],
>> +            ret = glfs_set_volfile_server(glfs, "tcp",
>>                                     server->value->u.tcp.host,
>>                                     (int)port);
>>          }
>> -- 
>> 2.7.4
>
> Instead of the strings for "unix" and "tcp", I would have liked
> #define's. Unfortunately it seems that these are not available in public
> headers :-/

Symbols from glfs.h would be ideal, yes.  But well-known string literals
aren't too bad.

> If this is easier to understand, I don't have any objections.

It's easier to read, but that's not my chief reason.  My chief reason is
that the values glfs_set_volfile_server() accepts are glfs's business,
while the names of the QAPI enumeration constants are QMP's business.
Coupling the two is inappropriate.

Decoupling them enables the renaming of the enumeration constant in
PATCH 14.

> Reviewed-by: Niels de Vos <ndevos@redhat.com>

Thanks!

[Qemu-devel] [PATCH 11/15] gluster: Don't duplicate qapi-util.c's qapi_enum_parse()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/gluster.c | 30 +++++++++---------------------
 1 file changed, 9 insertions(+), 21 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 7236d59..6fbcf9e 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -12,6 +12,7 @@
 #include "block/block_int.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qerror.h"
+#include "qapi/util.h"
 #include "qemu/uri.h"
 #include "qemu/error-report.h"
 #include "qemu/cutils.h"
@@ -472,23 +473,6 @@ out:
     return NULL;
 }
 
-static int qapi_enum_parse(const char *opt)
-{
-    int i;
-
-    if (!opt) {
-        return GLUSTER_TRANSPORT__MAX;
-    }
-
-    for (i = 0; i < GLUSTER_TRANSPORT__MAX; i++) {
-        if (!strcmp(opt, GlusterTransport_lookup[i])) {
-            return i;
-        }
-    }
-
-    return i;
-}
-
 /*
  * Convert the json formatted command line into qapi.
 */
@@ -546,16 +530,20 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
 
         ptr = qemu_opt_get(opts, GLUSTER_OPT_TYPE);
         gsconf = g_new0(GlusterServer, 1);
-        gsconf->type = qapi_enum_parse(ptr);
+        gsconf->type = qapi_enum_parse(GlusterTransport_lookup, ptr,
+                                       GLUSTER_TRANSPORT__MAX,
+                                       GLUSTER_TRANSPORT__MAX,
+                                       &local_err);
         if (!ptr) {
             error_setg(&local_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_TYPE);
             error_append_hint(&local_err, GERR_INDEX_HINT, i);
             goto out;
 
         }
-        if (gsconf->type == GLUSTER_TRANSPORT__MAX) {
-            error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE,
-                       GLUSTER_OPT_TYPE, "tcp or unix");
+        if (local_err) {
+            error_append_hint(&local_err,
+                              "Parameter '%s' may be 'tcp' or 'unix'\n",
+                              GLUSTER_OPT_TYPE);
             error_append_hint(&local_err, GERR_INDEX_HINT, i);
             goto out;
         }
-- 
2.7.4

Re: [Qemu-devel] [Qemu-block] [PATCH 11/15] gluster: Don't duplicate qapi-util.c's qapi_enum_parse()

[Niels de Vos]

On Thu, Mar 02, 2017 at 10:44:02PM +0100, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/gluster.c | 30 +++++++++---------------------
>  1 file changed, 9 insertions(+), 21 deletions(-)
> 
> diff --git a/block/gluster.c b/block/gluster.c
> index 7236d59..6fbcf9e 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -12,6 +12,7 @@
>  #include "block/block_int.h"
>  #include "qapi/error.h"
>  #include "qapi/qmp/qerror.h"
> +#include "qapi/util.h"
>  #include "qemu/uri.h"
>  #include "qemu/error-report.h"
>  #include "qemu/cutils.h"
> @@ -472,23 +473,6 @@ out:
>      return NULL;
>  }
>  
> -static int qapi_enum_parse(const char *opt)
> -{
> -    int i;
> -
> -    if (!opt) {
> -        return GLUSTER_TRANSPORT__MAX;
> -    }
> -
> -    for (i = 0; i < GLUSTER_TRANSPORT__MAX; i++) {
> -        if (!strcmp(opt, GlusterTransport_lookup[i])) {
> -            return i;
> -        }
> -    }
> -
> -    return i;
> -}
> -
>  /*
>   * Convert the json formatted command line into qapi.
>  */
> @@ -546,16 +530,20 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>  
>          ptr = qemu_opt_get(opts, GLUSTER_OPT_TYPE);
>          gsconf = g_new0(GlusterServer, 1);
> -        gsconf->type = qapi_enum_parse(ptr);
> +        gsconf->type = qapi_enum_parse(GlusterTransport_lookup, ptr,
> +                                       GLUSTER_TRANSPORT__MAX,
> +                                       GLUSTER_TRANSPORT__MAX,
> +                                       &local_err);
>          if (!ptr) {
>              error_setg(&local_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_TYPE);
>              error_append_hint(&local_err, GERR_INDEX_HINT, i);
>              goto out;
>  
>          }
> -        if (gsconf->type == GLUSTER_TRANSPORT__MAX) {
> -            error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE,
> -                       GLUSTER_OPT_TYPE, "tcp or unix");
> +        if (local_err) {
> +            error_append_hint(&local_err,
> +                              "Parameter '%s' may be 'tcp' or 'unix'\n",
> +                              GLUSTER_OPT_TYPE);
>              error_append_hint(&local_err, GERR_INDEX_HINT, i);
>              goto out;
>          }
> -- 
> 2.7.4
> 
> 

Looks good to me.

Reviewed-by: Niels de Vos <ndevos@redhat.com>

[Qemu-devel] [PATCH 12/15] gluster: Plug memory leaks in qemu_gluster_parse_json()

[Markus Armbruster]

To reproduce, run

    $ valgrind qemu-system-x86_64 --nodefaults -S --drive driver=gluster,volume=testvol,path=/a/b/c,server.0.type=xxx

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/gluster.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 6fbcf9e..35a7abb 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -480,7 +480,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
                                   QDict *options, Error **errp)
 {
     QemuOpts *opts;
-    GlusterServer *gsconf;
+    GlusterServer *gsconf = NULL;
     GlusterServerList *curr = NULL;
     QDict *backing_options = NULL;
     Error *local_err = NULL;
@@ -529,17 +529,16 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
         }
 
         ptr = qemu_opt_get(opts, GLUSTER_OPT_TYPE);
+        if (!ptr) {
+            error_setg(&local_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_TYPE);
+            error_append_hint(&local_err, GERR_INDEX_HINT, i);
+            goto out;
+
+        }
         gsconf = g_new0(GlusterServer, 1);
         gsconf->type = qapi_enum_parse(GlusterTransport_lookup, ptr,
-                                       GLUSTER_TRANSPORT__MAX,
-                                       GLUSTER_TRANSPORT__MAX,
+                                       GLUSTER_TRANSPORT__MAX, 0,
                                        &local_err);
-        if (!ptr) {
-            error_setg(&local_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_TYPE);
-            error_append_hint(&local_err, GERR_INDEX_HINT, i);
-            goto out;
-
-        }
         if (local_err) {
             error_append_hint(&local_err,
                               "Parameter '%s' may be 'tcp' or 'unix'\n",
@@ -626,8 +625,10 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
             curr->next->value = gsconf;
             curr = curr->next;
         }
+        gsconf = NULL;
 
-        qdict_del(backing_options, str);
+        QDECREF(backing_options);
+        backing_options = NULL;
         g_free(str);
         str = NULL;
     }
@@ -636,11 +637,10 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
 
 out:
     error_propagate(errp, local_err);
+    qapi_free_GlusterServer(gsconf);
     qemu_opts_del(opts);
-    if (str) {
-        qdict_del(backing_options, str);
-        g_free(str);
-    }
+    g_free(str);
+    QDECREF(backing_options);
     errno = EINVAL;
     return -errno;
 }
-- 
2.7.4

Re: [Qemu-devel] [Qemu-block] [PATCH 12/15] gluster: Plug memory leaks in qemu_gluster_parse_json()

[Niels de Vos]

On Thu, Mar 02, 2017 at 10:44:03PM +0100, Markus Armbruster wrote:
> To reproduce, run
> 
>     $ valgrind qemu-system-x86_64 --nodefaults -S --drive driver=gluster,volume=testvol,path=/a/b/c,server.0.type=xxx
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/gluster.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/block/gluster.c b/block/gluster.c
> index 6fbcf9e..35a7abb 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -480,7 +480,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>                                    QDict *options, Error **errp)
>  {
>      QemuOpts *opts;
> -    GlusterServer *gsconf;
> +    GlusterServer *gsconf = NULL;
>      GlusterServerList *curr = NULL;
>      QDict *backing_options = NULL;
>      Error *local_err = NULL;
> @@ -529,17 +529,16 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>          }
>  
>          ptr = qemu_opt_get(opts, GLUSTER_OPT_TYPE);
> +        if (!ptr) {
> +            error_setg(&local_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_TYPE);
> +            error_append_hint(&local_err, GERR_INDEX_HINT, i);
> +            goto out;
> +
> +        }
>          gsconf = g_new0(GlusterServer, 1);
>          gsconf->type = qapi_enum_parse(GlusterTransport_lookup, ptr,
> -                                       GLUSTER_TRANSPORT__MAX,
> -                                       GLUSTER_TRANSPORT__MAX,
> +                                       GLUSTER_TRANSPORT__MAX, 0,

What is the reason to set the default to 0 and not the more readable
GLUSTER_TRANSPORT_UNIX?

>                                         &local_err);
> -        if (!ptr) {
> -            error_setg(&local_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_TYPE);
> -            error_append_hint(&local_err, GERR_INDEX_HINT, i);
> -            goto out;
> -
> -        }
>          if (local_err) {
>              error_append_hint(&local_err,
>                                "Parameter '%s' may be 'tcp' or 'unix'\n",
> @@ -626,8 +625,10 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>              curr->next->value = gsconf;
>              curr = curr->next;
>          }
> +        gsconf = NULL;
>  
> -        qdict_del(backing_options, str);
> +        QDECREF(backing_options);
> +        backing_options = NULL;
>          g_free(str);
>          str = NULL;
>      }
> @@ -636,11 +637,10 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>  
>  out:
>      error_propagate(errp, local_err);
> +    qapi_free_GlusterServer(gsconf);
>      qemu_opts_del(opts);
> -    if (str) {
> -        qdict_del(backing_options, str);
> -        g_free(str);
> -    }
> +    g_free(str);
> +    QDECREF(backing_options);
>      errno = EINVAL;
>      return -errno;
>  }
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [Qemu-block] [PATCH 12/15] gluster: Plug memory leaks in qemu_gluster_parse_json()

[Markus Armbruster]

Niels de Vos <ndevos@redhat.com> writes:

> On Thu, Mar 02, 2017 at 10:44:03PM +0100, Markus Armbruster wrote:
>> To reproduce, run
>> 
>>     $ valgrind qemu-system-x86_64 --nodefaults -S --drive driver=gluster,volume=testvol,path=/a/b/c,server.0.type=xxx
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/gluster.c | 28 ++++++++++++++--------------
>>  1 file changed, 14 insertions(+), 14 deletions(-)
>> 
>> diff --git a/block/gluster.c b/block/gluster.c
>> index 6fbcf9e..35a7abb 100644
>> --- a/block/gluster.c
>> +++ b/block/gluster.c
>> @@ -480,7 +480,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>>                                    QDict *options, Error **errp)
>>  {
>>      QemuOpts *opts;
>> -    GlusterServer *gsconf;
>> +    GlusterServer *gsconf = NULL;
>>      GlusterServerList *curr = NULL;
>>      QDict *backing_options = NULL;
>>      Error *local_err = NULL;
>> @@ -529,17 +529,16 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>>          }
>>  
>>          ptr = qemu_opt_get(opts, GLUSTER_OPT_TYPE);
>> +        if (!ptr) {
>> +            error_setg(&local_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_TYPE);
>> +            error_append_hint(&local_err, GERR_INDEX_HINT, i);
>> +            goto out;
>> +
>> +        }
>>          gsconf = g_new0(GlusterServer, 1);
>>          gsconf->type = qapi_enum_parse(GlusterTransport_lookup, ptr,
>> -                                       GLUSTER_TRANSPORT__MAX,
>> -                                       GLUSTER_TRANSPORT__MAX,
>> +                                       GLUSTER_TRANSPORT__MAX, 0,
>
> What is the reason to set the default to 0 and not the more readable
> GLUSTER_TRANSPORT_UNIX?

I chose 0 because the actual value must not matter: we don't want a
default here.

qapi_enum_parse() returns this argument when ptr isn't found.  If ptr is
non-null, it additionally sets an error.  Since ptr can't be null here,
the argument is only returned together with an error.  But then we won't
use *gsconf.

Do you think -1 instead of 0 would be clearer?

>>                                         &local_err);
>> -        if (!ptr) {
>> -            error_setg(&local_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_TYPE);
>> -            error_append_hint(&local_err, GERR_INDEX_HINT, i);
>> -            goto out;
>> -
>> -        }
>>          if (local_err) {
>>              error_append_hint(&local_err,
>>                                "Parameter '%s' may be 'tcp' or 'unix'\n",
>> @@ -626,8 +625,10 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>>              curr->next->value = gsconf;
>>              curr = curr->next;
>>          }
>> +        gsconf = NULL;
>>  
>> -        qdict_del(backing_options, str);
>> +        QDECREF(backing_options);
>> +        backing_options = NULL;
>>          g_free(str);
>>          str = NULL;
>>      }
>> @@ -636,11 +637,10 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>>  
>>  out:
>>      error_propagate(errp, local_err);
>> +    qapi_free_GlusterServer(gsconf);
>>      qemu_opts_del(opts);
>> -    if (str) {
>> -        qdict_del(backing_options, str);
>> -        g_free(str);
>> -    }
>> +    g_free(str);
>> +    QDECREF(backing_options);
>>      errno = EINVAL;
>>      return -errno;
>>  }
>> -- 
>> 2.7.4
>> 
>> 

Re: [Qemu-devel] [Qemu-block] [PATCH 12/15] gluster: Plug memory leaks in qemu_gluster_parse_json()

[Niels de Vos]

On Fri, Mar 03, 2017 at 08:38:45AM +0100, Markus Armbruster wrote:
> Niels de Vos <ndevos@redhat.com> writes:
> 
> > On Thu, Mar 02, 2017 at 10:44:03PM +0100, Markus Armbruster wrote:
> >> To reproduce, run
> >> 
> >>     $ valgrind qemu-system-x86_64 --nodefaults -S --drive driver=gluster,volume=testvol,path=/a/b/c,server.0.type=xxx
> >> 
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  block/gluster.c | 28 ++++++++++++++--------------
> >>  1 file changed, 14 insertions(+), 14 deletions(-)
> >> 
> >> diff --git a/block/gluster.c b/block/gluster.c
> >> index 6fbcf9e..35a7abb 100644
> >> --- a/block/gluster.c
> >> +++ b/block/gluster.c
> >> @@ -480,7 +480,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
> >>                                    QDict *options, Error **errp)
> >>  {
> >>      QemuOpts *opts;
> >> -    GlusterServer *gsconf;
> >> +    GlusterServer *gsconf = NULL;
> >>      GlusterServerList *curr = NULL;
> >>      QDict *backing_options = NULL;
> >>      Error *local_err = NULL;
> >> @@ -529,17 +529,16 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
> >>          }
> >>  
> >>          ptr = qemu_opt_get(opts, GLUSTER_OPT_TYPE);
> >> +        if (!ptr) {
> >> +            error_setg(&local_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_TYPE);
> >> +            error_append_hint(&local_err, GERR_INDEX_HINT, i);
> >> +            goto out;
> >> +
> >> +        }
> >>          gsconf = g_new0(GlusterServer, 1);
> >>          gsconf->type = qapi_enum_parse(GlusterTransport_lookup, ptr,
> >> -                                       GLUSTER_TRANSPORT__MAX,
> >> -                                       GLUSTER_TRANSPORT__MAX,
> >> +                                       GLUSTER_TRANSPORT__MAX, 0,
> >
> > What is the reason to set the default to 0 and not the more readable
> > GLUSTER_TRANSPORT_UNIX?
> 
> I chose 0 because the actual value must not matter: we don't want a
> default here.
> 
> qapi_enum_parse() returns this argument when ptr isn't found.  If ptr is
> non-null, it additionally sets an error.  Since ptr can't be null here,
> the argument is only returned together with an error.  But then we won't
> use *gsconf.

Ah, right, I that see now.

> Do you think -1 instead of 0 would be clearer?

Yes, it would be to me. -1 is clearly not part of the GLUSTER_TRANSPORT_*
enum, so it suggests it is a different case.

Thanks!

> 
> >>                                         &local_err);
> >> -        if (!ptr) {
> >> -            error_setg(&local_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_TYPE);
> >> -            error_append_hint(&local_err, GERR_INDEX_HINT, i);
> >> -            goto out;
> >> -
> >> -        }
> >>          if (local_err) {
> >>              error_append_hint(&local_err,
> >>                                "Parameter '%s' may be 'tcp' or 'unix'\n",
> >> @@ -626,8 +625,10 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
> >>              curr->next->value = gsconf;
> >>              curr = curr->next;
> >>          }
> >> +        gsconf = NULL;
> >>  
> >> -        qdict_del(backing_options, str);
> >> +        QDECREF(backing_options);
> >> +        backing_options = NULL;
> >>          g_free(str);
> >>          str = NULL;
> >>      }
> >> @@ -636,11 +637,10 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
> >>  
> >>  out:
> >>      error_propagate(errp, local_err);
> >> +    qapi_free_GlusterServer(gsconf);
> >>      qemu_opts_del(opts);
> >> -    if (str) {
> >> -        qdict_del(backing_options, str);
> >> -        g_free(str);
> >> -    }
> >> +    g_free(str);
> >> +    QDECREF(backing_options);
> >>      errno = EINVAL;
> >>      return -errno;
> >>  }
> >> -- 
> >> 2.7.4
> >> 
> >> 

Re: [Qemu-devel] [Qemu-block] [PATCH 12/15] gluster: Plug memory leaks in qemu_gluster_parse_json()

[Markus Armbruster]

Niels de Vos <ndevos@redhat.com> writes:

> On Fri, Mar 03, 2017 at 08:38:45AM +0100, Markus Armbruster wrote:
>> Niels de Vos <ndevos@redhat.com> writes:
>> 
>> > On Thu, Mar 02, 2017 at 10:44:03PM +0100, Markus Armbruster wrote:
>> >> To reproduce, run
>> >> 
>> >>     $ valgrind qemu-system-x86_64 --nodefaults -S --drive driver=gluster,volume=testvol,path=/a/b/c,server.0.type=xxx
>> >> 
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >> ---
>> >>  block/gluster.c | 28 ++++++++++++++--------------
>> >>  1 file changed, 14 insertions(+), 14 deletions(-)
>> >> 
>> >> diff --git a/block/gluster.c b/block/gluster.c
>> >> index 6fbcf9e..35a7abb 100644
>> >> --- a/block/gluster.c
>> >> +++ b/block/gluster.c
>> >> @@ -480,7 +480,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>> >>                                    QDict *options, Error **errp)
>> >>  {
>> >>      QemuOpts *opts;
>> >> -    GlusterServer *gsconf;
>> >> +    GlusterServer *gsconf = NULL;
>> >>      GlusterServerList *curr = NULL;
>> >>      QDict *backing_options = NULL;
>> >>      Error *local_err = NULL;
>> >> @@ -529,17 +529,16 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>> >>          }
>> >>  
>> >>          ptr = qemu_opt_get(opts, GLUSTER_OPT_TYPE);
>> >> +        if (!ptr) {
>> >> +            error_setg(&local_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_TYPE);
>> >> +            error_append_hint(&local_err, GERR_INDEX_HINT, i);
>> >> +            goto out;
>> >> +
>> >> +        }
>> >>          gsconf = g_new0(GlusterServer, 1);
>> >>          gsconf->type = qapi_enum_parse(GlusterTransport_lookup, ptr,
>> >> -                                       GLUSTER_TRANSPORT__MAX,
>> >> -                                       GLUSTER_TRANSPORT__MAX,
>> >> +                                       GLUSTER_TRANSPORT__MAX, 0,
>> >
>> > What is the reason to set the default to 0 and not the more readable
>> > GLUSTER_TRANSPORT_UNIX?
>> 
>> I chose 0 because the actual value must not matter: we don't want a
>> default here.
>> 
>> qapi_enum_parse() returns this argument when ptr isn't found.  If ptr is
>> non-null, it additionally sets an error.  Since ptr can't be null here,
>> the argument is only returned together with an error.  But then we won't
>> use *gsconf.
>
> Ah, right, I that see now.
>
>> Do you think -1 instead of 0 would be clearer?
>
> Yes, it would be to me. -1 is clearly not part of the GLUSTER_TRANSPORT_*
> enum, so it suggests it is a different case.
>
> Thanks!

I'll change it.

May I add your R-by then?

Re: [Qemu-devel] [Qemu-block] [PATCH 12/15] gluster: Plug memory leaks in qemu_gluster_parse_json()

[Niels de Vos]

On Fri, Mar 03, 2017 at 09:35:16AM +0100, Markus Armbruster wrote:
> Niels de Vos <ndevos@redhat.com> writes:
> 
> > On Fri, Mar 03, 2017 at 08:38:45AM +0100, Markus Armbruster wrote:
> >> Niels de Vos <ndevos@redhat.com> writes:
> >> 
> >> > On Thu, Mar 02, 2017 at 10:44:03PM +0100, Markus Armbruster wrote:
> >> >> To reproduce, run
> >> >> 
> >> >>     $ valgrind qemu-system-x86_64 --nodefaults -S --drive driver=gluster,volume=testvol,path=/a/b/c,server.0.type=xxx
> >> >> 
> >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> >> ---
> >> >>  block/gluster.c | 28 ++++++++++++++--------------
> >> >>  1 file changed, 14 insertions(+), 14 deletions(-)
> >> >> 
> >> >> diff --git a/block/gluster.c b/block/gluster.c
> >> >> index 6fbcf9e..35a7abb 100644
> >> >> --- a/block/gluster.c
> >> >> +++ b/block/gluster.c
> >> >> @@ -480,7 +480,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
> >> >>                                    QDict *options, Error **errp)
> >> >>  {
> >> >>      QemuOpts *opts;
> >> >> -    GlusterServer *gsconf;
> >> >> +    GlusterServer *gsconf = NULL;
> >> >>      GlusterServerList *curr = NULL;
> >> >>      QDict *backing_options = NULL;
> >> >>      Error *local_err = NULL;
> >> >> @@ -529,17 +529,16 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
> >> >>          }
> >> >>  
> >> >>          ptr = qemu_opt_get(opts, GLUSTER_OPT_TYPE);
> >> >> +        if (!ptr) {
> >> >> +            error_setg(&local_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_TYPE);
> >> >> +            error_append_hint(&local_err, GERR_INDEX_HINT, i);
> >> >> +            goto out;
> >> >> +
> >> >> +        }
> >> >>          gsconf = g_new0(GlusterServer, 1);
> >> >>          gsconf->type = qapi_enum_parse(GlusterTransport_lookup, ptr,
> >> >> -                                       GLUSTER_TRANSPORT__MAX,
> >> >> -                                       GLUSTER_TRANSPORT__MAX,
> >> >> +                                       GLUSTER_TRANSPORT__MAX, 0,
> >> >
> >> > What is the reason to set the default to 0 and not the more readable
> >> > GLUSTER_TRANSPORT_UNIX?
> >> 
> >> I chose 0 because the actual value must not matter: we don't want a
> >> default here.
> >> 
> >> qapi_enum_parse() returns this argument when ptr isn't found.  If ptr is
> >> non-null, it additionally sets an error.  Since ptr can't be null here,
> >> the argument is only returned together with an error.  But then we won't
> >> use *gsconf.
> >
> > Ah, right, I that see now.
> >
> >> Do you think -1 instead of 0 would be clearer?
> >
> > Yes, it would be to me. -1 is clearly not part of the GLUSTER_TRANSPORT_*
> > enum, so it suggests it is a different case.
> >
> > Thanks!
> 
> I'll change it.
> 
> May I add your R-by then?

Yes, of course.

Thanks,
Niels

[Qemu-devel] [PATCH 13/15] qapi-schema: Rename GlusterServer to SocketAddressFlat

[Markus Armbruster]

As its documentation says, it's not specific to Gluster.  Rename it,
as I'm going to use it for something else.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/gluster.c      | 38 +++++++++++++++++++-------------------
 qapi-schema.json     | 38 ++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 46 +---------------------------------------------
 3 files changed, 58 insertions(+), 64 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 35a7abb..77ce45b 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -321,7 +321,7 @@ static int parse_volume_options(BlockdevOptionsGluster *gconf, char *path)
 static int qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf,
                                   const char *filename)
 {
-    GlusterServer *gsconf;
+    SocketAddressFlat *gsconf;
     URI *uri;
     QueryParams *qp = NULL;
     bool is_unix = false;
@@ -332,19 +332,19 @@ static int qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf,
         return -EINVAL;
     }
 
-    gconf->server = g_new0(GlusterServerList, 1);
-    gconf->server->value = gsconf = g_new0(GlusterServer, 1);
+    gconf->server = g_new0(SocketAddressFlatList, 1);
+    gconf->server->value = gsconf = g_new0(SocketAddressFlat, 1);
 
     /* transport */
     if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
-        gsconf->type = GLUSTER_TRANSPORT_TCP;
+        gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_TCP;
     } else if (!strcmp(uri->scheme, "gluster+tcp")) {
-        gsconf->type = GLUSTER_TRANSPORT_TCP;
+        gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_TCP;
     } else if (!strcmp(uri->scheme, "gluster+unix")) {
-        gsconf->type = GLUSTER_TRANSPORT_UNIX;
+        gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_UNIX;
         is_unix = true;
     } else if (!strcmp(uri->scheme, "gluster+rdma")) {
-        gsconf->type = GLUSTER_TRANSPORT_TCP;
+        gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_TCP;
         error_report("Warning: rdma feature is not supported, falling "
                      "back to tcp");
     } else {
@@ -396,7 +396,7 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
     struct glfs *glfs;
     int ret;
     int old_errno;
-    GlusterServerList *server;
+    SocketAddressFlatList *server;
     unsigned long long port;
 
     glfs = glfs_find_preopened(gconf->volume);
@@ -412,7 +412,7 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
     glfs_set_preopened(gconf->volume, glfs);
 
     for (server = gconf->server; server; server = server->next) {
-        if (server->value->type  == GLUSTER_TRANSPORT_UNIX) {
+        if (server->value->type  == SOCKET_ADDRESS_FLAT_TYPE_UNIX) {
             ret = glfs_set_volfile_server(glfs, "unix",
                                    server->value->u.q_unix.path, 0);
         } else {
@@ -443,7 +443,7 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
         error_setg(errp, "Gluster connection for volume %s, path %s failed"
                          " to connect", gconf->volume, gconf->path);
         for (server = gconf->server; server; server = server->next) {
-            if (server->value->type  == GLUSTER_TRANSPORT_UNIX) {
+            if (server->value->type  == SOCKET_ADDRESS_FLAT_TYPE_UNIX) {
                 error_append_hint(errp, "hint: failed on socket %s ",
                                   server->value->u.q_unix.path);
             } else {
@@ -480,8 +480,8 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
                                   QDict *options, Error **errp)
 {
     QemuOpts *opts;
-    GlusterServer *gsconf = NULL;
-    GlusterServerList *curr = NULL;
+    SocketAddressFlat *gsconf = NULL;
+    SocketAddressFlatList *curr = NULL;
     QDict *backing_options = NULL;
     Error *local_err = NULL;
     char *str = NULL;
@@ -535,9 +535,9 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
             goto out;
 
         }
-        gsconf = g_new0(GlusterServer, 1);
-        gsconf->type = qapi_enum_parse(GlusterTransport_lookup, ptr,
-                                       GLUSTER_TRANSPORT__MAX, 0,
+        gsconf = g_new0(SocketAddressFlat, 1);
+        gsconf->type = qapi_enum_parse(SocketAddressFlatType_lookup, ptr,
+                                       SOCKET_ADDRESS_FLAT_TYPE__MAX, 0,
                                        &local_err);
         if (local_err) {
             error_append_hint(&local_err,
@@ -548,7 +548,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
         }
         qemu_opts_del(opts);
 
-        if (gsconf->type == GLUSTER_TRANSPORT_TCP) {
+        if (gsconf->type == SOCKET_ADDRESS_FLAT_TYPE_TCP) {
             /* create opts info from runtime_tcp_opts list */
             opts = qemu_opts_create(&runtime_tcp_opts, NULL, 0, &error_abort);
             qemu_opts_absorb_qdict(opts, backing_options, &local_err);
@@ -617,11 +617,11 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
         }
 
         if (gconf->server == NULL) {
-            gconf->server = g_new0(GlusterServerList, 1);
+            gconf->server = g_new0(SocketAddressFlatList, 1);
             gconf->server->value = gsconf;
             curr = gconf->server;
         } else {
-            curr->next = g_new0(GlusterServerList, 1);
+            curr->next = g_new0(SocketAddressFlatList, 1);
             curr->next->value = gsconf;
             curr = curr->next;
         }
@@ -637,7 +637,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
 
 out:
     error_propagate(errp, local_err);
-    qapi_free_GlusterServer(gsconf);
+    qapi_free_SocketAddressFlat(gsconf);
     qemu_opts_del(opts);
     g_free(str);
     QDECREF(backing_options);
diff --git a/qapi-schema.json b/qapi-schema.json
index 150ee98..a57afeb 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4101,6 +4101,44 @@
     'fd': 'String' } }
 
 ##
+# @SocketAddressFlatType:
+#
+# Available SocketAddressFlat types
+#
+# @tcp:   Internet address
+#
+# @unix:  Unix domain socket
+#
+# Since: 2.9
+##
+{ 'enum': 'SocketAddressFlatType',
+  'data': [ 'unix', 'tcp' ] }
+
+##
+# @SocketAddressFlat:
+#
+# Captures the address of a socket
+#
+# @type:       Transport type
+#
+# This is similar to SocketAddress, only distinction:
+#
+# 1. SocketAddressFlat is a flat union, SocketAddress is a simple union.
+#    A flat union is nicer than simple because it avoids nesting
+#    (i.e. more {}) on the wire.
+#
+# 2. SocketAddressFlat supports only types 'unix' and 'tcp', because
+#    that's what its current users need.
+#
+# Since: 2.9
+##
+{ 'union': 'SocketAddressFlat',
+  'base': { 'type': 'SocketAddressFlatType' },
+  'discriminator': 'type',
+  'data': { 'unix': 'UnixSocketAddress',
+            'tcp': 'InetSocketAddress' } }
+
+##
 # @getfd:
 #
 # Receive a file descriptor via SCM rights and assign it a name
diff --git a/qapi/block-core.json b/qapi/block-core.json
index cf24c04..28f1b10 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2533,50 +2533,6 @@
             '*read-pattern': 'QuorumReadPattern' } }
 
 ##
-# @GlusterTransport:
-#
-# An enumeration of Gluster transport types
-#
-# @tcp:   TCP   - Transmission Control Protocol
-#
-# @unix:  UNIX  - Unix domain socket
-#
-# Since: 2.7
-##
-{ 'enum': 'GlusterTransport',
-  'data': [ 'unix', 'tcp' ] }
-
-
-##
-# @GlusterServer:
-#
-# Captures the address of a socket
-#
-# Details for connecting to a gluster server
-#
-# @type:       Transport type used for gluster connection
-#
-# This is similar to SocketAddress, only distinction:
-#
-# 1. GlusterServer is a flat union, SocketAddress is a simple union.
-#    A flat union is nicer than simple because it avoids nesting
-#    (i.e. more {}) on the wire.
-#
-# 2. GlusterServer lacks case 'fd', since gluster doesn't let you
-#    pass in a file descriptor.
-#
-# GlusterServer is actually not Gluster-specific, its a
-# compatibility evolved into an alternate for SocketAddress.
-#
-# Since: 2.7
-##
-{ 'union': 'GlusterServer',
-  'base': { 'type': 'GlusterTransport' },
-  'discriminator': 'type',
-  'data': { 'unix': 'UnixSocketAddress',
-            'tcp': 'InetSocketAddress' } }
-
-##
 # @BlockdevOptionsGluster:
 #
 # Driver specific block device options for Gluster
@@ -2597,7 +2553,7 @@
 { 'struct': 'BlockdevOptionsGluster',
   'data': { 'volume': 'str',
             'path': 'str',
-            'server': ['GlusterServer'],
+            'server': ['SocketAddressFlat'],
             '*debug': 'int',
             '*logfile': 'str' } }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH 13/15] qapi-schema: Rename GlusterServer to SocketAddressFlat

[Eric Blake]

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

On 03/02/2017 03:44 PM, Markus Armbruster wrote:
> As its documentation says, it's not specific to Gluster.  Rename it,
> as I'm going to use it for something else.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/gluster.c      | 38 +++++++++++++++++++-------------------
>  qapi-schema.json     | 38 ++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json | 46 +---------------------------------------------
>  3 files changed, 58 insertions(+), 64 deletions(-)

Mostly mechanical once you deal with the cross-file motion.  Changing
the type name has no impact on the wire representation, so it's safe.

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

> +++ b/qapi-schema.json
> @@ -4101,6 +4101,44 @@
>      'fd': 'String' } }
>  
>  ##
> +# @SocketAddressFlatType:
> +#
> +# Available SocketAddressFlat types
> +#
> +# @tcp:   Internet address
> +#
> +# @unix:  Unix domain socket
> +#
> +# Since: 2.9

I probably would have listed 'since: 2.7', since the type is unchanged
from its pre-move location...

> +#
> +# Since: 2.9
> +##
> +{ 'union': 'SocketAddressFlat',

and again


> +++ b/qapi/block-core.json
> @@ -2533,50 +2533,6 @@
>              '*read-pattern': 'QuorumReadPattern' } }
>  
>  ##
> -# @GlusterTransport:
> -#
> -# An enumeration of Gluster transport types
> -#
> -# @tcp:   TCP   - Transmission Control Protocol
> -#
> -# @unix:  UNIX  - Unix domain socket
> -#
> -# Since: 2.7

...here.

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


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

Re: [Qemu-devel] [PATCH 13/15] qapi-schema: Rename GlusterServer to SocketAddressFlat

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/02/2017 03:44 PM, Markus Armbruster wrote:
>> As its documentation says, it's not specific to Gluster.  Rename it,
>> as I'm going to use it for something else.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/gluster.c      | 38 +++++++++++++++++++-------------------
>>  qapi-schema.json     | 38 ++++++++++++++++++++++++++++++++++++++
>>  qapi/block-core.json | 46 +---------------------------------------------
>>  3 files changed, 58 insertions(+), 64 deletions(-)
>
> Mostly mechanical once you deal with the cross-file motion.  Changing
> the type name has no impact on the wire representation, so it's safe.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
>> +++ b/qapi-schema.json
>> @@ -4101,6 +4101,44 @@
>>      'fd': 'String' } }
>>  
>>  ##
>> +# @SocketAddressFlatType:
>> +#
>> +# Available SocketAddressFlat types
>> +#
>> +# @tcp:   Internet address
>> +#
>> +# @unix:  Unix domain socket
>> +#
>> +# Since: 2.9
>
> I probably would have listed 'since: 2.7', since the type is unchanged
> from its pre-move location...

I'd have to bump it right in the next patch :)

>> +#
>> +# Since: 2.9
>> +##
>> +{ 'union': 'SocketAddressFlat',
>
> and again
>
>
>> +++ b/qapi/block-core.json
>> @@ -2533,50 +2533,6 @@
>>              '*read-pattern': 'QuorumReadPattern' } }
>>  
>>  ##
>> -# @GlusterTransport:
>> -#
>> -# An enumeration of Gluster transport types
>> -#
>> -# @tcp:   TCP   - Transmission Control Protocol
>> -#
>> -# @unix:  UNIX  - Unix domain socket
>> -#
>> -# Since: 2.7
>
> ...here.

Re: [Qemu-devel] [PATCH 13/15] qapi-schema: Rename GlusterServer to SocketAddressFlat

[Eric Blake]

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

On 03/03/2017 11:05 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 

>>> +# @SocketAddressFlatType:
>>> +#
>>> +# Available SocketAddressFlat types
>>> +#
>>> +# @tcp:   Internet address
>>> +#
>>> +# @unix:  Unix domain socket
>>> +#
>>> +# Since: 2.9
>>
>> I probably would have listed 'since: 2.7', since the type is unchanged
>> from its pre-move location...
> 
> I'd have to bump it right in the next patch :)

Too true (now that I've read the next patch)!  No need to tweak it, then.

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


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

[Qemu-devel] [PATCH 14/15] qapi-schema: Rename SocketAddressFlat's variant tcp to inet

[Markus Armbruster]

QAPI type SocketAddressFlat differs from SocketAddress pointlessly:
the discriminator value for variant InetSocketAddress is 'tcp' instead
of 'inet'.  Rename.

The type is far only used by the Gluster block drivers.  Take care to
keep 'tcp' working there.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/gluster.c  | 59 +++++++++++++++++++++++++++++---------------------------
 qapi-schema.json |  8 ++++----
 2 files changed, 35 insertions(+), 32 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 77ce45b..0155188 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -152,7 +152,7 @@ static QemuOptsList runtime_type_opts = {
         {
             .name = GLUSTER_OPT_TYPE,
             .type = QEMU_OPT_STRING,
-            .help = "tcp|unix",
+            .help = "inet|unix",
         },
         { /* end of list */ }
     },
@@ -171,14 +171,14 @@ static QemuOptsList runtime_unix_opts = {
     },
 };
 
-static QemuOptsList runtime_tcp_opts = {
-    .name = "gluster_tcp",
-    .head = QTAILQ_HEAD_INITIALIZER(runtime_tcp_opts.head),
+static QemuOptsList runtime_inet_opts = {
+    .name = "gluster_inet",
+    .head = QTAILQ_HEAD_INITIALIZER(runtime_inet_opts.head),
     .desc = {
         {
             .name = GLUSTER_OPT_TYPE,
             .type = QEMU_OPT_STRING,
-            .help = "tcp|unix",
+            .help = "inet|unix",
         },
         {
             .name = GLUSTER_OPT_HOST,
@@ -337,14 +337,14 @@ static int qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf,
 
     /* transport */
     if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
-        gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_TCP;
+        gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_INET;
     } else if (!strcmp(uri->scheme, "gluster+tcp")) {
-        gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_TCP;
+        gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_INET;
     } else if (!strcmp(uri->scheme, "gluster+unix")) {
         gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_UNIX;
         is_unix = true;
     } else if (!strcmp(uri->scheme, "gluster+rdma")) {
-        gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_TCP;
+        gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_INET;
         error_report("Warning: rdma feature is not supported, falling "
                      "back to tcp");
     } else {
@@ -374,11 +374,11 @@ static int qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf,
         }
         gsconf->u.q_unix.path = g_strdup(qp->p[0].value);
     } else {
-        gsconf->u.tcp.host = g_strdup(uri->server ? uri->server : "localhost");
+        gsconf->u.inet.host = g_strdup(uri->server ? uri->server : "localhost");
         if (uri->port) {
-            gsconf->u.tcp.port = g_strdup_printf("%d", uri->port);
+            gsconf->u.inet.port = g_strdup_printf("%d", uri->port);
         } else {
-            gsconf->u.tcp.port = g_strdup_printf("%d", GLUSTER_DEFAULT_PORT);
+            gsconf->u.inet.port = g_strdup_printf("%d", GLUSTER_DEFAULT_PORT);
         }
     }
 
@@ -416,15 +416,15 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
             ret = glfs_set_volfile_server(glfs, "unix",
                                    server->value->u.q_unix.path, 0);
         } else {
-            if (parse_uint_full(server->value->u.tcp.port, &port, 10) < 0 ||
+            if (parse_uint_full(server->value->u.inet.port, &port, 10) < 0 ||
                 port > 65535) {
                 error_setg(errp, "'%s' is not a valid port number",
-                           server->value->u.tcp.port);
+                           server->value->u.inet.port);
                 errno = EINVAL;
                 goto out;
             }
             ret = glfs_set_volfile_server(glfs, "tcp",
-                                   server->value->u.tcp.host,
+                                   server->value->u.inet.host,
                                    (int)port);
         }
 
@@ -448,8 +448,8 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
                                   server->value->u.q_unix.path);
             } else {
                 error_append_hint(errp, "hint: failed on host %s and port %s ",
-                                  server->value->u.tcp.host,
-                                  server->value->u.tcp.port);
+                                  server->value->u.inet.host,
+                                  server->value->u.inet.port);
             }
         }
 
@@ -536,21 +536,24 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
 
         }
         gsconf = g_new0(SocketAddressFlat, 1);
+        if (!strcmp(ptr, "tcp")) {
+            ptr = "inet";       /* accept legacy "tcp" */
+        }
         gsconf->type = qapi_enum_parse(SocketAddressFlatType_lookup, ptr,
                                        SOCKET_ADDRESS_FLAT_TYPE__MAX, 0,
                                        &local_err);
         if (local_err) {
             error_append_hint(&local_err,
-                              "Parameter '%s' may be 'tcp' or 'unix'\n",
+                              "Parameter '%s' may be 'inet' or 'unix'\n",
                               GLUSTER_OPT_TYPE);
             error_append_hint(&local_err, GERR_INDEX_HINT, i);
             goto out;
         }
         qemu_opts_del(opts);
 
-        if (gsconf->type == SOCKET_ADDRESS_FLAT_TYPE_TCP) {
-            /* create opts info from runtime_tcp_opts list */
-            opts = qemu_opts_create(&runtime_tcp_opts, NULL, 0, &error_abort);
+        if (gsconf->type == SOCKET_ADDRESS_FLAT_TYPE_INET) {
+            /* create opts info from runtime_inet_opts list */
+            opts = qemu_opts_create(&runtime_inet_opts, NULL, 0, &error_abort);
             qemu_opts_absorb_qdict(opts, backing_options, &local_err);
             if (local_err) {
                 goto out;
@@ -563,7 +566,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
                 error_append_hint(&local_err, GERR_INDEX_HINT, i);
                 goto out;
             }
-            gsconf->u.tcp.host = g_strdup(ptr);
+            gsconf->u.inet.host = g_strdup(ptr);
             ptr = qemu_opt_get(opts, GLUSTER_OPT_PORT);
             if (!ptr) {
                 error_setg(&local_err, QERR_MISSING_PARAMETER,
@@ -571,28 +574,28 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
                 error_append_hint(&local_err, GERR_INDEX_HINT, i);
                 goto out;
             }
-            gsconf->u.tcp.port = g_strdup(ptr);
+            gsconf->u.inet.port = g_strdup(ptr);
 
             /* defend for unsupported fields in InetSocketAddress,
              * i.e. @ipv4, @ipv6  and @to
              */
             ptr = qemu_opt_get(opts, GLUSTER_OPT_TO);
             if (ptr) {
-                gsconf->u.tcp.has_to = true;
+                gsconf->u.inet.has_to = true;
             }
             ptr = qemu_opt_get(opts, GLUSTER_OPT_IPV4);
             if (ptr) {
-                gsconf->u.tcp.has_ipv4 = true;
+                gsconf->u.inet.has_ipv4 = true;
             }
             ptr = qemu_opt_get(opts, GLUSTER_OPT_IPV6);
             if (ptr) {
-                gsconf->u.tcp.has_ipv6 = true;
+                gsconf->u.inet.has_ipv6 = true;
             }
-            if (gsconf->u.tcp.has_to) {
+            if (gsconf->u.inet.has_to) {
                 error_setg(&local_err, "Parameter 'to' not supported");
                 goto out;
             }
-            if (gsconf->u.tcp.has_ipv4 || gsconf->u.tcp.has_ipv6) {
+            if (gsconf->u.inet.has_ipv4 || gsconf->u.inet.has_ipv6) {
                 error_setg(&local_err, "Parameters 'ipv4/ipv6' not supported");
                 goto out;
             }
@@ -669,7 +672,7 @@ static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf,
                              "file.volume=testvol,file.path=/path/a.qcow2"
                              "[,file.debug=9]"
                              "[,file.logfile=/path/filename.log],"
-                             "file.server.0.type=tcp,"
+                             "file.server.0.type=inet,"
                              "file.server.0.host=1.2.3.4,"
                              "file.server.0.port=24007,"
                              "file.server.1.transport=unix,"
diff --git a/qapi-schema.json b/qapi-schema.json
index a57afeb..dfaebce 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4105,14 +4105,14 @@
 #
 # Available SocketAddressFlat types
 #
-# @tcp:   Internet address
+# @inet:   Internet address
 #
 # @unix:  Unix domain socket
 #
 # Since: 2.9
 ##
 { 'enum': 'SocketAddressFlatType',
-  'data': [ 'unix', 'tcp' ] }
+  'data': [ 'unix', 'inet' ] }
 
 ##
 # @SocketAddressFlat:
@@ -4127,7 +4127,7 @@
 #    A flat union is nicer than simple because it avoids nesting
 #    (i.e. more {}) on the wire.
 #
-# 2. SocketAddressFlat supports only types 'unix' and 'tcp', because
+# 2. SocketAddressFlat supports only types 'unix' and 'inet', because
 #    that's what its current users need.
 #
 # Since: 2.9
@@ -4136,7 +4136,7 @@
   'base': { 'type': 'SocketAddressFlatType' },
   'discriminator': 'type',
   'data': { 'unix': 'UnixSocketAddress',
-            'tcp': 'InetSocketAddress' } }
+            'inet': 'InetSocketAddress' } }
 
 ##
 # @getfd:
-- 
2.7.4

Re: [Qemu-devel] [PATCH 14/15] qapi-schema: Rename SocketAddressFlat's variant tcp to inet

[Eric Blake]

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

On 03/02/2017 03:44 PM, Markus Armbruster wrote:
> QAPI type SocketAddressFlat differs from SocketAddress pointlessly:
> the discriminator value for variant InetSocketAddress is 'tcp' instead
> of 'inet'.  Rename.
> 
> The type is far only used by the Gluster block drivers.  Take care to
> keep 'tcp' working there.

The old name was visible in QMP in 2.8, but only by blockdev-add, which
we've already argued was not stable (and where we've already made other
non-back-compat changes to it).  But that means this HAS to go into 2.9,
if we're declaring blockdev-add stable for 2.9.

It wouldn't hurt to mention that additional information in the commit
message.

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/gluster.c  | 59 +++++++++++++++++++++++++++++---------------------------
>  qapi-schema.json |  8 ++++----
>  2 files changed, 35 insertions(+), 32 deletions(-)
> 

> +++ b/qapi-schema.json
> @@ -4105,14 +4105,14 @@
>  #
>  # Available SocketAddressFlat types
>  #
> -# @tcp:   Internet address
> +# @inet:   Internet address
>  #
>  # @unix:  Unix domain socket

Nit: Spacing is now inconsistent.

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

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


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

Re: [Qemu-devel] [PATCH 14/15] qapi-schema: Rename SocketAddressFlat's variant tcp to inet

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/02/2017 03:44 PM, Markus Armbruster wrote:
>> QAPI type SocketAddressFlat differs from SocketAddress pointlessly:
>> the discriminator value for variant InetSocketAddress is 'tcp' instead
>> of 'inet'.  Rename.
>> 
>> The type is far only used by the Gluster block drivers.  Take care to
>> keep 'tcp' working there.
>
> The old name was visible in QMP in 2.8, but only by blockdev-add, which
> we've already argued was not stable (and where we've already made other
> non-back-compat changes to it).  But that means this HAS to go into 2.9,
> if we're declaring blockdev-add stable for 2.9.

Yes.

Note that the command line pseudo-filename's URI syntax stays the same
(file=gluster+tcp://), and the command line's dotted key syntax keeps
accepting tcp for compatiblity (file.server.0.type=tcp works in addition
to =inet).

> It wouldn't hurt to mention that additional information in the commit
> message.

I'll cook something up.

>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/gluster.c  | 59 +++++++++++++++++++++++++++++---------------------------
>>  qapi-schema.json |  8 ++++----
>>  2 files changed, 35 insertions(+), 32 deletions(-)
>> 
>> diff --git a/block/gluster.c b/block/gluster.c
>> index 77ce45b..0155188 100644
>> --- a/block/gluster.c
>> +++ b/block/gluster.c
[...]
>> @@ -536,21 +536,24 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>>  
>>          }
>>          gsconf = g_new0(SocketAddressFlat, 1);
>> +        if (!strcmp(ptr, "tcp")) {
>> +            ptr = "inet";       /* accept legacy "tcp" */
>> +        }
>>          gsconf->type = qapi_enum_parse(SocketAddressFlatType_lookup, ptr,
>>                                         SOCKET_ADDRESS_FLAT_TYPE__MAX, 0,
>>                                         &local_err);

This is where I keep file.server.N.type=tcp working.

[...]
>> +++ b/qapi-schema.json
>> @@ -4105,14 +4105,14 @@
>>  #
>>  # Available SocketAddressFlat types
>>  #
>> -# @tcp:   Internet address
>> +# @inet:   Internet address
>>  #
>>  # @unix:  Unix domain socket
>
> Nit: Spacing is now inconsistent.

Will fix.

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

Thanks!

Re: [Qemu-devel] [PATCH 14/15] qapi-schema: Rename SocketAddressFlat's variant tcp to inet

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Eric Blake <eblake@redhat.com> writes:
>
>> On 03/02/2017 03:44 PM, Markus Armbruster wrote:
>>> QAPI type SocketAddressFlat differs from SocketAddress pointlessly:
>>> the discriminator value for variant InetSocketAddress is 'tcp' instead
>>> of 'inet'.  Rename.
>>> 
>>> The type is far only used by the Gluster block drivers.  Take care to
>>> keep 'tcp' working there.
>>
>> The old name was visible in QMP in 2.8, but only by blockdev-add, which
>> we've already argued was not stable (and where we've already made other
>> non-back-compat changes to it).  But that means this HAS to go into 2.9,
>> if we're declaring blockdev-add stable for 2.9.
>
> Yes.
>
> Note that the command line pseudo-filename's URI syntax stays the same
> (file=gluster+tcp://), and the command line's dotted key syntax keeps
> accepting tcp for compatiblity (file.server.0.type=tcp works in addition
> to =inet).
>
>> It wouldn't hurt to mention that additional information in the commit
>> message.
>
> I'll cook something up.

Replacing the second paragraph by

    The type is so far only used by the Gluster block drivers.  Take care
    to keep 'tcp' working in things like -drive's file.server.0.type=tcp.
    The "gluster+tcp" URI scheme in pseudo-filenames stays the same.
    blockdev-add changes, but it has changed incompatibly since 2.8
    already.

[...]

[Qemu-devel] [PATCH 15/15] sheepdog: Support blockdev-add

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qapi/block-core.json | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 28f1b10..3b1e4ad 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2111,6 +2111,7 @@
 # @replication: Since 2.8
 # @ssh: Since 2.8
 # @iscsi: Since 2.9
+# @sheepdog: Since 2.9
 #
 # Since: 2.0
 ##
@@ -2119,8 +2120,8 @@
             'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
             'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs',
             'null-aio', 'null-co', 'parallels', 'qcow', 'qcow2', 'qed',
-            'quorum', 'raw', 'replication', 'ssh', 'vdi', 'vhdx', 'vmdk',
-            'vpc', 'vvfat' ] }
+            'quorum', 'raw', 'replication', 'sheepdog', 'ssh', 'vdi',
+            'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
 
 ##
 # @BlockdevOptionsFile:
@@ -2622,6 +2623,26 @@
             '*timeout': 'int' } }
 
 ##
+# @BlockdevOptionsSheepdog:
+#
+# Driver specific block device options for sheepdog
+#
+# @vdi:         Virtual disk image name
+# @addr:        The Sheepdog server to connect to
+# @snap-id:     Snapshot ID
+# @tag:         Snapshot tag name
+#
+# Only one of @snap-id and @tag may be present.
+#
+# Since: 2.9
+##
+{ 'struct': 'BlockdevOptionsSheepdog',
+  'data': { 'addr': 'SocketAddressFlat',
+            'vdi': 'str',
+            '*snap-id': 'uint32',
+            '*tag': 'str' } }
+
+##
 # @ReplicationMode:
 #
 # An enumeration of replication modes.
@@ -2821,7 +2842,7 @@
       'raw':        'BlockdevOptionsRaw',
 # TODO rbd: Wait for structured options
       'replication':'BlockdevOptionsReplication',
-# TODO sheepdog: Wait for structured options
+      'sheepdog':   'BlockdevOptionsSheepdog',
       'ssh':        'BlockdevOptionsSsh',
       'vdi':        'BlockdevOptionsGenericFormat',
       'vhdx':       'BlockdevOptionsGenericFormat',
-- 
2.7.4

Re: [Qemu-devel] [PATCH 15/15] sheepdog: Support blockdev-add

[Eric Blake]

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

On 03/02/2017 03:44 PM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qapi/block-core.json | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 

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

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


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

Re: [Qemu-devel] [PATCH 00/15] block: A bunch of fixes for Sheepdog and Gluster

[Eric Blake]

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

On 03/02/2017 03:43 PM, Markus Armbruster wrote:
> Bad error handling, memory leaks, and lack of blockdev-add support.

How hard are we trying to get blockdev-add working in 2.9?  Or is this
series 2.10 material now?

> 
> Markus Armbruster (15):
>   sheepdog: Defuse time bomb in sd_open() error handling
>   sheepdog: Fix error handling in sd_snapshot_delete()
>   sheepdog: Fix error handling sd_create()
>   sheepdog: Mark sd_snapshot_delete() lossage FIXME
>   sheepdog: Fix snapshot ID parsing in _open(), _create, _goto()
>   sheepdog: Don't truncate long VDI name in _open(), _create()
>   sheepdog: Report errors in pseudo-filename more usefully
>   sheepdog: Use SocketAddress and socket_connect()
>   sheepdog: Implement bdrv_parse_filename()
>   gluster: Drop assumptions on SocketTransport names
>   gluster: Don't duplicate qapi-util.c's qapi_enum_parse()
>   gluster: Plug memory leaks in qemu_gluster_parse_json()
>   qapi-schema: Rename GlusterServer to SocketAddressFlat
>   qapi-schema: Rename SocketAddressFlat's variant tcp to inet
>   sheepdog: Support blockdev-add
> 
>  block/gluster.c      | 127 +++++++--------
>  block/sheepdog.c     | 436 +++++++++++++++++++++++++++++++++++++--------------
>  qapi-schema.json     |  38 +++++
>  qapi/block-core.json |  73 +++------
>  4 files changed, 443 insertions(+), 231 deletions(-)
> 

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


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

Re: [Qemu-devel] [PATCH 00/15] block: A bunch of fixes for Sheepdog and Gluster

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/02/2017 03:43 PM, Markus Armbruster wrote:
>> Bad error handling, memory leaks, and lack of blockdev-add support.
>
> How hard are we trying to get blockdev-add working in 2.9?  Or is this
> series 2.10 material now?

Definitely not 2.10: seven patches fix or document bugs, one improves
error messages, five are straightforward cleanups.  The series touches
only these two block drivers, including QAPI schema parts not used
anywhere else:

>>  block/gluster.c      | 127 +++++++--------
>>  block/sheepdog.c     | 436 +++++++++++++++++++++++++++++++++++++--------------
>>  qapi-schema.json     |  38 +++++
>>  qapi/block-core.json |  73 +++------
>>  4 files changed, 443 insertions(+), 231 deletions(-)

With the pending pull requests merged, blockdev-add *is* working, except
for sheepdog.  I'm considering that a bug, and I need *two* patches to
fix it.  One touches only sheepdog.c, and the other only adds to the
QAPI schema.

If we decide not to fix the bug, I'd recommend to declare blockdev-add
supported in 2.9 anyway, with a release note that sheepdog support is
broken.

Re: [Qemu-devel] [PATCH 00/15] block: A bunch of fixes for Sheepdog and Gluster

[Eric Blake]

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

On 03/02/2017 11:39 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 03/02/2017 03:43 PM, Markus Armbruster wrote:
>>> Bad error handling, memory leaks, and lack of blockdev-add support.
>>
>> How hard are we trying to get blockdev-add working in 2.9?  Or is this
>> series 2.10 material now?
> 
> Definitely not 2.10: seven patches fix or document bugs, one improves
> error messages, five are straightforward cleanups.  The series touches
> only these two block drivers, including QAPI schema parts not used
> anywhere else:
> 
>>>  block/gluster.c      | 127 +++++++--------
>>>  block/sheepdog.c     | 436 +++++++++++++++++++++++++++++++++++++--------------
>>>  qapi-schema.json     |  38 +++++
>>>  qapi/block-core.json |  73 +++------
>>>  4 files changed, 443 insertions(+), 231 deletions(-)
> 
> With the pending pull requests merged, blockdev-add *is* working, except
> for sheepdog.  I'm considering that a bug, and I need *two* patches to
> fix it.  One touches only sheepdog.c, and the other only adds to the
> QAPI schema.

I can agree to that. It may help if the v2 is explicit that the series
is for-2.9 in the subject.

> 
> If we decide not to fix the bug, I'd recommend to declare blockdev-add
> supported in 2.9 anyway, with a release note that sheepdog support is
> broken.
> 

Yes, I think we can still treat the rename of 'x-blockdev-del' to
'blockdev-del' as a bug fix appropriate for 2.9, finally declaring the
feature stable.

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


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

Re: [Qemu-devel] [PATCH 00/15] block: A bunch of fixes for Sheepdog and Gluster

[Peter Maydell]

On 2 March 2017 at 21:43, Markus Armbruster <armbru@redhat.com> wrote:
> Bad error handling, memory leaks, and lack of blockdev-add support.
>
> Markus Armbruster (15):
>   sheepdog: Defuse time bomb in sd_open() error handling
>   sheepdog: Fix error handling in sd_snapshot_delete()
>   sheepdog: Fix error handling sd_create()
>   sheepdog: Mark sd_snapshot_delete() lossage FIXME
>   sheepdog: Fix snapshot ID parsing in _open(), _create, _goto()
>   sheepdog: Don't truncate long VDI name in _open(), _create()
>   sheepdog: Report errors in pseudo-filename more usefully
>   sheepdog: Use SocketAddress and socket_connect()
>   sheepdog: Implement bdrv_parse_filename()
>   gluster: Drop assumptions on SocketTransport names
>   gluster: Don't duplicate qapi-util.c's qapi_enum_parse()
>   gluster: Plug memory leaks in qemu_gluster_parse_json()
>   qapi-schema: Rename GlusterServer to SocketAddressFlat
>   qapi-schema: Rename SocketAddressFlat's variant tcp to inet
>   sheepdog: Support blockdev-add
>
>  block/gluster.c      | 127 +++++++--------
>  block/sheepdog.c     | 436 +++++++++++++++++++++++++++++++++++++--------------
>  qapi-schema.json     |  38 +++++
>  qapi/block-core.json |  73 +++------
>  4 files changed, 443 insertions(+), 231 deletions(-)

Did you find these by looking at our Coverity results, or
cross-reference them against Coverity? (Coverity definitely
found the gluster leaks, for instance.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 00/15] block: A bunch of fixes for Sheepdog and Gluster

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 2 March 2017 at 21:43, Markus Armbruster <armbru@redhat.com> wrote:
>> Bad error handling, memory leaks, and lack of blockdev-add support.
>>
>> Markus Armbruster (15):
>>   sheepdog: Defuse time bomb in sd_open() error handling
>>   sheepdog: Fix error handling in sd_snapshot_delete()
>>   sheepdog: Fix error handling sd_create()
>>   sheepdog: Mark sd_snapshot_delete() lossage FIXME
>>   sheepdog: Fix snapshot ID parsing in _open(), _create, _goto()
>>   sheepdog: Don't truncate long VDI name in _open(), _create()
>>   sheepdog: Report errors in pseudo-filename more usefully
>>   sheepdog: Use SocketAddress and socket_connect()
>>   sheepdog: Implement bdrv_parse_filename()
>>   gluster: Drop assumptions on SocketTransport names
>>   gluster: Don't duplicate qapi-util.c's qapi_enum_parse()
>>   gluster: Plug memory leaks in qemu_gluster_parse_json()
>>   qapi-schema: Rename GlusterServer to SocketAddressFlat
>>   qapi-schema: Rename SocketAddressFlat's variant tcp to inet
>>   sheepdog: Support blockdev-add
>>
>>  block/gluster.c      | 127 +++++++--------
>>  block/sheepdog.c     | 436 +++++++++++++++++++++++++++++++++++++--------------
>>  qapi-schema.json     |  38 +++++
>>  qapi/block-core.json |  73 +++------
>>  4 files changed, 443 insertions(+), 231 deletions(-)
>
> Did you find these by looking at our Coverity results, or
> cross-reference them against Coverity? (Coverity definitely
> found the gluster leaks, for instance.)

Neither.  I merely hacked myself a path through an unfamliar jungle :)

Re: [Qemu-devel] [PATCH 00/15] block: A bunch of fixes for Sheepdog and Gluster

[Peter Maydell]

On 3 March 2017 at 18:37, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> Did you find these by looking at our Coverity results, or
>> cross-reference them against Coverity? (Coverity definitely
>> found the gluster leaks, for instance.)
>
> Neither.  I merely hacked myself a path through an unfamliar jungle :)

OK; I guess we'll see how many of the coverity nits get fixed :-)

thanks
-- PMM