6:19 a.m.
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(a)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
7:27 a.m.
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(a)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;
8:39 p.m.
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(a)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
1:22 a.m.
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
2:58 a.m.
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
4:18 a.m.
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
4:21 a.m.
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
4138
days inactive
4139
days old
6 comments
4 participants
participants (4)
-
Eric Blake -
Ján Tomko -
Martin Kletzander -
Michal Privoznik