[Qemu-devel] [PATCH for-2.12] gluster: Fix blockdev-add with server.N.type=unix

[Qemu-devel] [PATCH for-2.12] gluster: Fix blockdev-add with server.N.type=unix

[Kevin Wolf]

The legacy command line interface gets the socket path from an option
called 'socket'. QAPI in contract uses SocketAddress, where the
corresponding option is called 'path'.

Fix the gluster block driver to accept both 'socket' and 'path', with
'path' being the preferred syntax.

https://bugzilla.redhat.com/show_bug.cgi?id=1545155

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/gluster.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 296e036b3d..4adc1a875b 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -167,7 +167,12 @@ static QemuOptsList runtime_unix_opts = {
         {
             .name = GLUSTER_OPT_SOCKET,
             .type = QEMU_OPT_STRING,
-            .help = "socket file path)",
+            .help = "socket file path (legacy)",
+        },
+        {
+            .name = GLUSTER_OPT_PATH,
+            .type = QEMU_OPT_STRING,
+            .help = "socket file path (QAPI)",
         },
         { /* end of list */ }
     },
@@ -615,10 +620,18 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
                 goto out;
             }
 
-            ptr = qemu_opt_get(opts, GLUSTER_OPT_SOCKET);
+            ptr = qemu_opt_get(opts, GLUSTER_OPT_PATH);
+            if (!ptr) {
+                ptr = qemu_opt_get(opts, GLUSTER_OPT_SOCKET);
+            } else if (qemu_opt_get(opts, GLUSTER_OPT_SOCKET)) {
+                error_setg(&local_err,
+                           "Conflicting parameters 'path' and 'socket'");
+                error_append_hint(&local_err, GERR_INDEX_HINT, i);
+                goto out;
+            }
             if (!ptr) {
                 error_setg(&local_err, QERR_MISSING_PARAMETER,
-                           GLUSTER_OPT_SOCKET);
+                           GLUSTER_OPT_PATH);
                 error_append_hint(&local_err, GERR_INDEX_HINT, i);
                 goto out;
             }
@@ -684,7 +697,7 @@ static int qemu_gluster_parse(BlockdevOptionsGluster *gconf,
                              "file.server.0.host=1.2.3.4,"
                              "file.server.0.port=24007,"
                              "file.server.1.transport=unix,"
-                             "file.server.1.socket=/var/run/glusterd.socket ..."
+                             "file.server.1.path=/var/run/glusterd.socket ..."
                              "\n");
             return ret;
         }
-- 
2.13.6

Re: [Qemu-devel] [PATCH for-2.12] gluster: Fix blockdev-add with server.N.type=unix

[Eric Blake]

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

On 04/03/2018 06:08 AM, Kevin Wolf wrote:
> The legacy command line interface gets the socket path from an option
> called 'socket'. QAPI in contract uses SocketAddress, where the
> corresponding option is called 'path'.
> 
> Fix the gluster block driver to accept both 'socket' and 'path', with
> 'path' being the preferred syntax.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1545155
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/gluster.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)

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

Should we also add a deprecation warning for 'socket' and update the
deprecation documentation, so we can start the clock ticking on getting
rid of maintaining the back-compat glue forever?

> @@ -615,10 +620,18 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>                  goto out;
>              }
>  
> -            ptr = qemu_opt_get(opts, GLUSTER_OPT_SOCKET);
> +            ptr = qemu_opt_get(opts, GLUSTER_OPT_PATH);
> +            if (!ptr) {
> +                ptr = qemu_opt_get(opts, GLUSTER_OPT_SOCKET);

Here's where we'd warn, if we want to deprecate things.

> +            } else if (qemu_opt_get(opts, GLUSTER_OPT_SOCKET)) {
> +                error_setg(&local_err,
> +                           "Conflicting parameters 'path' and 'socket'");
> +                error_append_hint(&local_err, GERR_INDEX_HINT, i);
> +                goto out;
> +            }
>              if (!ptr) {

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.12] gluster: Fix blockdev-add with server.N.type=unix

[Peter Krempa]

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

On Tue, Apr 03, 2018 at 08:09:49 -0500, Eric Blake wrote:
> On 04/03/2018 06:08 AM, Kevin Wolf wrote:
> > The legacy command line interface gets the socket path from an option
> > called 'socket'. QAPI in contract uses SocketAddress, where the
> > corresponding option is called 'path'.
> > 
> > Fix the gluster block driver to accept both 'socket' and 'path', with
> > 'path' being the preferred syntax.
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1545155
> > 
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/gluster.c | 21 +++++++++++++++++----
> >  1 file changed, 17 insertions(+), 4 deletions(-)

Thanks for fixing this. I'm using the new syntax in the -blockdev code
in libvirt and since I'm qapi- schema-checking the results, it would
not be possible to use the legacy approach.

> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> Should we also add a deprecation warning for 'socket' and update the
> deprecation documentation, so we can start the clock ticking on getting
> rid of maintaining the back-compat glue forever?

Well, that won't be as easy. Since there is at least one qemu release
which declared this in the QAPI schema but did not support using it it's
hard for libvirt to detect that this was fixed, and thus we can't infer
a capability which would be used to switch to the new-syntax only.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.12] gluster: Fix blockdev-add with server.N.type=unix

[Kevin Wolf]

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

Am 04.04.2018 um 17:41 hat Peter Krempa geschrieben:
> On Tue, Apr 03, 2018 at 08:09:49 -0500, Eric Blake wrote:
> > On 04/03/2018 06:08 AM, Kevin Wolf wrote:
> > > The legacy command line interface gets the socket path from an option
> > > called 'socket'. QAPI in contract uses SocketAddress, where the
> > > corresponding option is called 'path'.
> > > 
> > > Fix the gluster block driver to accept both 'socket' and 'path', with
> > > 'path' being the preferred syntax.
> > > 
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1545155
> > > 
> > > Cc: qemu-stable@nongnu.org
> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > ---
> > >  block/gluster.c | 21 +++++++++++++++++----
> > >  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> Thanks for fixing this. I'm using the new syntax in the -blockdev code
> in libvirt and since I'm qapi- schema-checking the results, it would
> not be possible to use the legacy approach.

-blockdev is validated against the schema on the QEMU side, too. The
legacy option can only be used with -drive.

> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > 
> > Should we also add a deprecation warning for 'socket' and update the
> > deprecation documentation, so we can start the clock ticking on getting
> > rid of maintaining the back-compat glue forever?
> 
> Well, that won't be as easy. Since there is at least one qemu release
> which declared this in the QAPI schema but did not support using it it's
> hard for libvirt to detect that this was fixed, and thus we can't infer
> a capability which would be used to switch to the new-syntax only.

Markus wanted to add some way to declare capabilities for a command in
the schema that could be queried with schema introspection and would
cover such cases. I think we even discussed this with you at KVM Forum?

Eric, do you happen to know if there were any patches on the list
related to this?

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.12] gluster: Fix blockdev-add with server.N.type=unix

[Eric Blake]

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

On 04/04/2018 10:54 AM, Kevin Wolf wrote:

>>> Should we also add a deprecation warning for 'socket' and update the
>>> deprecation documentation, so we can start the clock ticking on getting
>>> rid of maintaining the back-compat glue forever?
>>
>> Well, that won't be as easy. Since there is at least one qemu release
>> which declared this in the QAPI schema but did not support using it it's
>> hard for libvirt to detect that this was fixed, and thus we can't infer
>> a capability which would be used to switch to the new-syntax only.
> 
> Markus wanted to add some way to declare capabilities for a command in
> the schema that could be queried with schema introspection and would
> cover such cases. I think we even discussed this with you at KVM Forum?
> 
> Eric, do you happen to know if there were any patches on the list
> related to this?

I don't recall seeing any patches from Markus before his leave, and
doubt anyone else has tackled anything along these lines.  It's too late
for 2.12, at any rate, so even if we had such a feature implemented in
time for 2.13, it won't help introspecting the 2.12 release.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.12] gluster: Fix blockdev-add with server.N.type=unix

[Kevin Wolf]

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

Am 04.04.2018 um 20:16 hat Eric Blake geschrieben:
> On 04/04/2018 10:54 AM, Kevin Wolf wrote:
> 
> >>> Should we also add a deprecation warning for 'socket' and update the
> >>> deprecation documentation, so we can start the clock ticking on getting
> >>> rid of maintaining the back-compat glue forever?
> >>
> >> Well, that won't be as easy. Since there is at least one qemu release
> >> which declared this in the QAPI schema but did not support using it it's
> >> hard for libvirt to detect that this was fixed, and thus we can't infer
> >> a capability which would be used to switch to the new-syntax only.
> > 
> > Markus wanted to add some way to declare capabilities for a command in
> > the schema that could be queried with schema introspection and would
> > cover such cases. I think we even discussed this with you at KVM Forum?
> > 
> > Eric, do you happen to know if there were any patches on the list
> > related to this?
> 
> I don't recall seeing any patches from Markus before his leave, and
> doubt anyone else has tackled anything along these lines.  It's too late
> for 2.12, at any rate, so even if we had such a feature implemented in
> time for 2.13, it won't help introspecting the 2.12 release.

We only strictly need it in introspection at the time when we remove the
old option. But maybe it would be nicer to wait with the deprecation
error_report() until libvirt has a chance to avoid the warning.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.12] gluster: Fix blockdev-add with server.N.type=unix

[Eric Blake]

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

On 04/04/2018 01:37 PM, Kevin Wolf wrote:
> Am 04.04.2018 um 20:16 hat Eric Blake geschrieben:
>> On 04/04/2018 10:54 AM, Kevin Wolf wrote:
>>
>>>>> Should we also add a deprecation warning for 'socket' and update the
>>>>> deprecation documentation, so we can start the clock ticking on getting
>>>>> rid of maintaining the back-compat glue forever?
>>>>

> We only strictly need it in introspection at the time when we remove the
> old option. But maybe it would be nicer to wait with the deprecation
> error_report() until libvirt has a chance to avoid the warning.

Makes sense - so no deprecation warning for 2.12, and the timeline for
performing a deprecation is shifted by a couple of releases after the
first time we introduce proper introspection support.  Which makes this
patch fine as-is.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH for-2.12] gluster: Fix blockdev-add with server.N.type=unix

[Jeff Cody]

On Tue, Apr 03, 2018 at 01:08:10PM +0200, Kevin Wolf wrote:
> The legacy command line interface gets the socket path from an option
> called 'socket'. QAPI in contract uses SocketAddress, where the
> corresponding option is called 'path'.
> 
> Fix the gluster block driver to accept both 'socket' and 'path', with
> 'path' being the preferred syntax.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1545155
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  block/gluster.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/block/gluster.c b/block/gluster.c
> index 296e036b3d..4adc1a875b 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -167,7 +167,12 @@ static QemuOptsList runtime_unix_opts = {
>          {
>              .name = GLUSTER_OPT_SOCKET,
>              .type = QEMU_OPT_STRING,
> -            .help = "socket file path)",
> +            .help = "socket file path (legacy)",
> +        },
> +        {
> +            .name = GLUSTER_OPT_PATH,
> +            .type = QEMU_OPT_STRING,
> +            .help = "socket file path (QAPI)",
>          },
>          { /* end of list */ }
>      },
> @@ -615,10 +620,18 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>                  goto out;
>              }
>  
> -            ptr = qemu_opt_get(opts, GLUSTER_OPT_SOCKET);
> +            ptr = qemu_opt_get(opts, GLUSTER_OPT_PATH);
> +            if (!ptr) {
> +                ptr = qemu_opt_get(opts, GLUSTER_OPT_SOCKET);
> +            } else if (qemu_opt_get(opts, GLUSTER_OPT_SOCKET)) {
> +                error_setg(&local_err,
> +                           "Conflicting parameters 'path' and 'socket'");
> +                error_append_hint(&local_err, GERR_INDEX_HINT, i);
> +                goto out;
> +            }
>              if (!ptr) {
>                  error_setg(&local_err, QERR_MISSING_PARAMETER,
> -                           GLUSTER_OPT_SOCKET);
> +                           GLUSTER_OPT_PATH);
>                  error_append_hint(&local_err, GERR_INDEX_HINT, i);
>                  goto out;
>              }
> @@ -684,7 +697,7 @@ static int qemu_gluster_parse(BlockdevOptionsGluster *gconf,
>                               "file.server.0.host=1.2.3.4,"
>                               "file.server.0.port=24007,"
>                               "file.server.1.transport=unix,"
> -                             "file.server.1.socket=/var/run/glusterd.socket ..."
> +                             "file.server.1.path=/var/run/glusterd.socket ..."
>                               "\n");
>              return ret;
>          }
> -- 
> 2.13.6
> 

Re: [Qemu-devel] [PATCH for-2.12] gluster: Fix blockdev-add with server.N.type=unix

[Jeff Cody]

On Tue, Apr 03, 2018 at 01:08:10PM +0200, Kevin Wolf wrote:
> The legacy command line interface gets the socket path from an option
> called 'socket'. QAPI in contract uses SocketAddress, where the
> corresponding option is called 'path'.
> 
> Fix the gluster block driver to accept both 'socket' and 'path', with
> 'path' being the preferred syntax.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1545155
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/gluster.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/block/gluster.c b/block/gluster.c
> index 296e036b3d..4adc1a875b 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -167,7 +167,12 @@ static QemuOptsList runtime_unix_opts = {
>          {
>              .name = GLUSTER_OPT_SOCKET,
>              .type = QEMU_OPT_STRING,
> -            .help = "socket file path)",
> +            .help = "socket file path (legacy)",
> +        },
> +        {
> +            .name = GLUSTER_OPT_PATH,
> +            .type = QEMU_OPT_STRING,
> +            .help = "socket file path (QAPI)",
>          },
>          { /* end of list */ }
>      },
> @@ -615,10 +620,18 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>                  goto out;
>              }
>  
> -            ptr = qemu_opt_get(opts, GLUSTER_OPT_SOCKET);
> +            ptr = qemu_opt_get(opts, GLUSTER_OPT_PATH);
> +            if (!ptr) {
> +                ptr = qemu_opt_get(opts, GLUSTER_OPT_SOCKET);
> +            } else if (qemu_opt_get(opts, GLUSTER_OPT_SOCKET)) {
> +                error_setg(&local_err,
> +                           "Conflicting parameters 'path' and 'socket'");
> +                error_append_hint(&local_err, GERR_INDEX_HINT, i);
> +                goto out;
> +            }
>              if (!ptr) {
>                  error_setg(&local_err, QERR_MISSING_PARAMETER,
> -                           GLUSTER_OPT_SOCKET);
> +                           GLUSTER_OPT_PATH);
>                  error_append_hint(&local_err, GERR_INDEX_HINT, i);
>                  goto out;
>              }
> @@ -684,7 +697,7 @@ static int qemu_gluster_parse(BlockdevOptionsGluster *gconf,
>                               "file.server.0.host=1.2.3.4,"
>                               "file.server.0.port=24007,"
>                               "file.server.1.transport=unix,"
> -                             "file.server.1.socket=/var/run/glusterd.socket ..."
> +                             "file.server.1.path=/var/run/glusterd.socket ..."
>                               "\n");
>              return ret;
>          }
> -- 
> 2.13.6
> 

Thanks,

Applied to my block branch:

git://github.com/codyprime/qemu-kvm-jtc block

-Jeff