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
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
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.
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
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
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
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
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
>
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
© 2016 - 2024 Red Hat, Inc.