[libvirt] [PATCH] virsh: Fix debugging
Commit a0b6a36f "fixed" what abfff210 broke (URI precedence), but there was still one more thing missing to fix. When using virsh parameters to setup debugging, those weren't honored, because at the time debugging was initializing, arguments weren't parsed yet. To make ewerything work as expected, we need to initialize the debugging twice, once before debugging (so we can debug option parsing properly) and then again after these options are parsed. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tools/virsh.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tools/virsh.c b/tools/virsh.c index 2ea44a6..ac77156 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2333,6 +2333,10 @@ vshInitDebug(vshControl *ctl) static bool vshInit(vshControl *ctl) { + /* Since we have the commandline arguments parsed, we need to + * re-initialize all the debugging to make it work properly */ + vshInitDebug(ctl); + if (ctl->conn) return false; -- 1.8.3.2
On 08/27/2013 01:19 PM, Martin Kletzander wrote:
Commit a0b6a36f "fixed" what abfff210 broke (URI precedence), but there was still one more thing missing to fix. When using virsh parameters to setup debugging, those weren't honored, because at the time debugging was initializing, arguments weren't parsed yet. To make ewerything work as expected, we need to initialize the debugging twice, once before debugging (so we can debug option parsing properly) and then again after these options are parsed.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> ---
As Ján pointed out in person, this fix (apart from the leak it introduces) does use all the settings configured, i.e.: 1) it parses VIRSH_DEBUG and VIRSH_LOG_FILE 2) it parses command line arguments the output behaves according to (1) 3) and after that, it uses debug settings from command line. But this was intended from my side as it is pretty reasonable IMHO. Anyway, feel free to express your opinions on how do we fix this (what's the desired behavior) and I'll cook up an appropriate patch. If nobody replies (cares), I'll send v2 or in case of ACK mentioned here, I can push it with this squashed in: diff --git a/tools/virsh.c b/tools/virsh.c index ac77156..34f5c4a 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2321,10 +2321,9 @@ vshInitDebug(vshControl *ctl) debugEnv = getenv("VIRSH_LOG_FILE"); if (debugEnv && *debugEnv) { ctl->logfile = vshStrdup(ctl, debugEnv); + vshOpenLogFile(ctl); } } - - vshOpenLogFile(ctl); } /* @@ -3044,7 +3043,9 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) ctl->readonly = true; break; case 'l': + vshCloseLogFile(ctl); ctl->logfile = vshStrdup(ctl, optarg); + vshOpenLogFile(ctl); break; case 'e': len = strlen(optarg); --- Martin
tools/virsh.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/tools/virsh.c b/tools/virsh.c index 2ea44a6..ac77156 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2333,6 +2333,10 @@ vshInitDebug(vshControl *ctl) static bool vshInit(vshControl *ctl) { + /* Since we have the commandline arguments parsed, we need to + * re-initialize all the debugging to make it work properly */ + vshInitDebug(ctl); + if (ctl->conn) return false;
On 08/27/2013 06:27 AM, Martin Kletzander wrote:
On 08/27/2013 01:19 PM, Martin Kletzander wrote:
Commit a0b6a36f "fixed" what abfff210 broke (URI precedence), but there was still one more thing missing to fix. When using virsh parameters to setup debugging, those weren't honored, because at the time debugging was initializing, arguments weren't parsed yet. To make ewerything work as expected, we need to initialize the debugging twice, once before debugging (so we can debug option parsing properly) and then again after these options are parsed.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> ---
As Ján pointed out in person, this fix (apart from the leak it introduces) does use all the settings configured, i.e.: 1) it parses VIRSH_DEBUG and VIRSH_LOG_FILE 2) it parses command line arguments the output behaves according to (1) 3) and after that, it uses debug settings from command line.
But this was intended from my side as it is pretty reasonable IMHO. Anyway, feel free to express your opinions on how do we fix this (what's the desired behavior) and I'll cook up an appropriate patch. If nobody replies (cares), I'll send v2 or in case of ACK mentioned here, I can push it with this squashed in:
I can live with the patch as amended by this squash. ACK.
diff --git a/tools/virsh.c b/tools/virsh.c index ac77156..34f5c4a 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2321,10 +2321,9 @@ vshInitDebug(vshControl *ctl) debugEnv = getenv("VIRSH_LOG_FILE"); if (debugEnv && *debugEnv) { ctl->logfile = vshStrdup(ctl, debugEnv); + vshOpenLogFile(ctl); } } - - vshOpenLogFile(ctl); }
/* @@ -3044,7 +3043,9 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) ctl->readonly = true; break; case 'l': + vshCloseLogFile(ctl); ctl->logfile = vshStrdup(ctl, optarg); + vshOpenLogFile(ctl);
Note that there's another leak here (pre-existing): If I call: virsh -l file1 -l file2, we leak "file1" by reassigning a malloc'd string without first freeing the old version. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/28/2013 03:39 AM, Eric Blake wrote:
On 08/27/2013 06:27 AM, Martin Kletzander wrote:
diff --git a/tools/virsh.c b/tools/virsh.c index ac77156..34f5c4a 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2321,10 +2321,9 @@ vshInitDebug(vshControl *ctl) debugEnv = getenv("VIRSH_LOG_FILE"); if (debugEnv && *debugEnv) { ctl->logfile = vshStrdup(ctl, debugEnv); + vshOpenLogFile(ctl); } } - - vshOpenLogFile(ctl); }
/* @@ -3044,7 +3043,9 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) ctl->readonly = true; break; case 'l': + vshCloseLogFile(ctl); ctl->logfile = vshStrdup(ctl, optarg); + vshOpenLogFile(ctl);
Note that there's another leak here (pre-existing): If I call: virsh -l file1 -l file2, we leak "file1" by reassigning a malloc'd string without first freeing the old version.
Actually, it gets freed in vshCloseLogFile (and set to NULL, just to be double sure): if (ctl->logfile) { VIR_FREE(ctl->logfile); ctl->logfile = NULL; } Jan
On 08/28/2013 08:22 AM, Ján Tomko wrote:
On 08/28/2013 03:39 AM, Eric Blake wrote:
On 08/27/2013 06:27 AM, Martin Kletzander wrote:
diff --git a/tools/virsh.c b/tools/virsh.c index ac77156..34f5c4a 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2321,10 +2321,9 @@ vshInitDebug(vshControl *ctl) debugEnv = getenv("VIRSH_LOG_FILE"); if (debugEnv && *debugEnv) { ctl->logfile = vshStrdup(ctl, debugEnv); + vshOpenLogFile(ctl); } } - - vshOpenLogFile(ctl); }
/* @@ -3044,7 +3043,9 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) ctl->readonly = true; break; case 'l': + vshCloseLogFile(ctl); ctl->logfile = vshStrdup(ctl, optarg); + vshOpenLogFile(ctl);
Note that there's another leak here (pre-existing): If I call: virsh -l file1 -l file2, we leak "file1" by reassigning a malloc'd string without first freeing the old version.
Actually, it gets freed in vshCloseLogFile (and set to NULL, just to be double sure):
if (ctl->logfile) { VIR_FREE(ctl->logfile); ctl->logfile = NULL; }
Yes it does. I amended the patch, added a line in the commit message mentioning this leak is fixed as a side effect and pushed. Thanks for the reviews, Martin
On 28.08.2013 08:22, Ján Tomko wrote:
On 08/28/2013 03:39 AM, Eric Blake wrote:
On 08/27/2013 06:27 AM, Martin Kletzander wrote:
diff --git a/tools/virsh.c b/tools/virsh.c index ac77156..34f5c4a 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2321,10 +2321,9 @@ vshInitDebug(vshControl *ctl) debugEnv = getenv("VIRSH_LOG_FILE"); if (debugEnv && *debugEnv) { ctl->logfile = vshStrdup(ctl, debugEnv); + vshOpenLogFile(ctl); } } - - vshOpenLogFile(ctl); }
/* @@ -3044,7 +3043,9 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) ctl->readonly = true; break; case 'l': + vshCloseLogFile(ctl); ctl->logfile = vshStrdup(ctl, optarg); + vshOpenLogFile(ctl);
Note that there's another leak here (pre-existing): If I call: virsh -l file1 -l file2, we leak "file1" by reassigning a malloc'd string without first freeing the old version.
Actually, it gets freed in vshCloseLogFile (and set to NULL, just to be double sure):
if (ctl->logfile) { VIR_FREE(ctl->logfile); ctl->logfile = NULL;
BTW this makes no sense, since VIR_FREE() sets every passed pointer to NULL. so this assignment is just statement without any effect. Michal
On Wed 28 Aug 2013 11:18:43 AM CEST, Michal Privoznik wrote:
On 28.08.2013 08:22, Ján Tomko wrote:
On 08/28/2013 03:39 AM, Eric Blake wrote:
On 08/27/2013 06:27 AM, Martin Kletzander wrote:
diff --git a/tools/virsh.c b/tools/virsh.c index ac77156..34f5c4a 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2321,10 +2321,9 @@ vshInitDebug(vshControl *ctl) debugEnv = getenv("VIRSH_LOG_FILE"); if (debugEnv && *debugEnv) { ctl->logfile = vshStrdup(ctl, debugEnv); + vshOpenLogFile(ctl); } } - - vshOpenLogFile(ctl); }
/* @@ -3044,7 +3043,9 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) ctl->readonly = true; break; case 'l': + vshCloseLogFile(ctl); ctl->logfile = vshStrdup(ctl, optarg); + vshOpenLogFile(ctl);
Note that there's another leak here (pre-existing): If I call: virsh -l file1 -l file2, we leak "file1" by reassigning a malloc'd string without first freeing the old version.
Actually, it gets freed in vshCloseLogFile (and set to NULL, just to be double sure):
if (ctl->logfile) { VIR_FREE(ctl->logfile); ctl->logfile = NULL;
BTW this makes no sense, since VIR_FREE() sets every passed pointer to NULL. so this assignment is just statement without any effect.
Moreover if ctl->logfile is NULL, it won't do anything, so we can save 3 lines by just doing VIR_FREE without the condition and NULL-reset, yes. But that's unrelated to the patch ;-) Martin
participants (4)
-
Eric Blake -
Ján Tomko -
Martin Kletzander -
Michal Privoznik