[libvirt] [PATCH] Fix URI connect precedence
Commit abfff210 changed the order of vshParseArgv() and vshInit() in order to make fix debugging of parameter parsing. However, vshInit() did a vshReconnect() even though ctl->name wasn't set according to the '-c' parameter yet. In order to keep both issues fixed, I've split the vshInit() into vshInitDebug() and vshInit(). One simple memleak of ctl->name is fixed as a part of this patch, since it is related to the issue it's fixing. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=999323 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tools/virsh.c | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 15f529e..2ea44a6 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2294,16 +2294,13 @@ vshEventLoop(void *opaque) /* - * Initialize connection. + * Initialize debug settings. */ -static bool -vshInit(vshControl *ctl) +static void +vshInitDebug(vshControl *ctl) { char *debugEnv; - if (ctl->conn) - return false; - if (ctl->debug == VSH_DEBUG_DEFAULT) { /* log level not set from commandline, check env variable */ debugEnv = getenv("VIRSH_DEBUG"); @@ -2328,6 +2325,16 @@ vshInit(vshControl *ctl) } vshOpenLogFile(ctl); +} + +/* + * Initialize connection. + */ +static bool +vshInit(vshControl *ctl) +{ + if (ctl->conn) + return false; /* set up the library error handler */ virSetErrorFunc(NULL, virshErrorHandler); @@ -3017,6 +3024,7 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) ctl->timing = true; break; case 'c': + VIR_FREE(ctl->name); ctl->name = vshStrdup(ctl, optarg); break; case 'v': @@ -3192,12 +3200,10 @@ main(int argc, char **argv) ctl->name = vshStrdup(ctl, defaultConn); } - if (!vshInit(ctl)) { - vshDeinit(ctl); - exit(EXIT_FAILURE); - } + vshInitDebug(ctl); - if (!vshParseArgv(ctl, argc, argv)) { + if (!vshParseArgv(ctl, argc, argv) || + !vshInit(ctl)) { vshDeinit(ctl); exit(EXIT_FAILURE); } -- 1.8.3.2
On Wed, Aug 21, 2013 at 11:15:39AM +0200, Martin Kletzander wrote:
Commit abfff210 changed the order of vshParseArgv() and vshInit() in order to make fix debugging of parameter parsing. However, vshInit() did a vshReconnect() even though ctl->name wasn't set according to the '-c' parameter yet. In order to keep both issues fixed, I've split the vshInit() into vshInitDebug() and vshInit().
One simple memleak of ctl->name is fixed as a part of this patch, since it is related to the issue it's fixing.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=999323 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tools/virsh.c | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-)
This looks like something we can usefully test for. eg create a test suite that does something like this URI1=test:///$(top_srcdir)/examples/test/testnode.xml URI2=test:///$(top_srcdir)/examples/test/testnodeinline.xml export LIBVIRT_DEFAULT_URI=$URI1 GOTURI=`virsh -c $URI2 uri` if $GOTURI == $URI2 ....pass... else ...fail... fi
diff --git a/tools/virsh.c b/tools/virsh.c index 15f529e..2ea44a6 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2294,16 +2294,13 @@ vshEventLoop(void *opaque)
/* - * Initialize connection. + * Initialize debug settings. */ -static bool -vshInit(vshControl *ctl) +static void +vshInitDebug(vshControl *ctl) { char *debugEnv;
- if (ctl->conn) - return false; - if (ctl->debug == VSH_DEBUG_DEFAULT) { /* log level not set from commandline, check env variable */ debugEnv = getenv("VIRSH_DEBUG"); @@ -2328,6 +2325,16 @@ vshInit(vshControl *ctl) }
vshOpenLogFile(ctl); +} + +/* + * Initialize connection. + */ +static bool +vshInit(vshControl *ctl) +{ + if (ctl->conn) + return false;
/* set up the library error handler */ virSetErrorFunc(NULL, virshErrorHandler); @@ -3017,6 +3024,7 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) ctl->timing = true; break; case 'c': + VIR_FREE(ctl->name); ctl->name = vshStrdup(ctl, optarg); break; case 'v': @@ -3192,12 +3200,10 @@ main(int argc, char **argv) ctl->name = vshStrdup(ctl, defaultConn); }
- if (!vshInit(ctl)) { - vshDeinit(ctl); - exit(EXIT_FAILURE); - } + vshInitDebug(ctl);
- if (!vshParseArgv(ctl, argc, argv)) { + if (!vshParseArgv(ctl, argc, argv) || + !vshInit(ctl)) { vshDeinit(ctl); exit(EXIT_FAILURE); }
ACK -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 08/21/2013 12:00 PM, Daniel P. Berrange wrote:
On Wed, Aug 21, 2013 at 11:15:39AM +0200, Martin Kletzander wrote:
Commit abfff210 changed the order of vshParseArgv() and vshInit() in order to make fix debugging of parameter parsing. However, vshInit() did a vshReconnect() even though ctl->name wasn't set according to the '-c' parameter yet. In order to keep both issues fixed, I've split the vshInit() into vshInitDebug() and vshInit().
One simple memleak of ctl->name is fixed as a part of this patch, since it is related to the issue it's fixing.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=999323 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tools/virsh.c | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-)
This looks like something we can usefully test for.
eg create a test suite that does something like this
URI1=test:///$(top_srcdir)/examples/test/testnode.xml URI2=test:///$(top_srcdir)/examples/test/testnodeinline.xml
export LIBVIRT_DEFAULT_URI=$URI1 GOTURI=`virsh -c $URI2 uri`
if $GOTURI == $URI2 ....pass... else ...fail... fi
Thanks for the tip, I'll also add all possible settings for URIs in the test to make sure we don't break anything else as well. [...]
ACK
Thanks, I pushed this and will send the test for review after I polish it a bit. Martin
participants (2)
-
Daniel P. Berrange -
Martin Kletzander