gluster: Fix blockdev-add with server.N.type=unix

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 [email protected]

Signed-off-by Kevin Wolf <[email protected]>

---
 block/gluster.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

block/gluster.c +6 -0

+            } 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) {

Quoted 7 lines · 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.

Quoted 7 lines

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

Cc [email protected]

Signed-off-by Kevin Wolf <[email protected]>

---
 block/gluster.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

Reviewed-by Eric Blake <[email protected]>

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?

Quoted 8 lines · Kevin Wolf

Quoted 7 lines

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.

Quoted 7 lines

Quoted 7 lines

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

Cc [email protected]

Signed-off-by Kevin Wolf <[email protected]>

---
 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.

Quoted 5 lines

Reviewed-by Eric Blake <[email protected]>

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.

Quoted 8 lines

Quoted 3 lines

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?

Quoted 14 lines

Quoted 9 lines

Quoted 4 lines

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?

Quoted 2 lines

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.

Am 04.04.2018 um 20:16 hat Eric Blake geschrieben:

Quoted 6 lines · Kevin Wolf

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

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.

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 [email protected]

Signed-off-by Kevin Wolf <[email protected]>

---
 block/gluster.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

block/gluster.c +17 -4

index 296e036b3d..4adc1a875b 100644
@@ -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;
         }

Signature · 1 line

2.13.6

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 [email protected]

Signed-off-by Kevin Wolf <[email protected]>

---
 block/gluster.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

block/gluster.c +17 -4

index 296e036b3d..4adc1a875b 100644
@@ -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;
         }

Signature · 1 line

2.13.6