Patchwork [08/11] qemu-log: Plug trivial memory leak in cpu_set_log_filename()

login
register
mail settings
Submitter Markus Armbruster
Date Jan. 16, 2013, 2:36 p.m.
Message ID <1358346998-26328-9-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/212538/
State New
Headers show

Comments

Markus Armbruster - Jan. 16, 2013, 2:36 p.m.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qemu-log.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)
Eric Blake - Jan. 16, 2013, 3:53 p.m.
On 01/16/2013 07:36 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qemu-log.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/qemu-log.c b/qemu-log.c
> index 64a1b88..1eadf1b 100644
> --- a/qemu-log.c
> +++ b/qemu-log.c
> @@ -21,10 +21,12 @@
>  #include "qemu/log.h"
>  
>  #ifdef WIN32
> -static const char *logfilename = "qemu.log";
> +#define DEFAULT_LOGFILENAME = "qemu.log"

Syntax error.

> @@ -54,11 +56,13 @@ void qemu_log_mask(int mask, const char *fmt, ...)
>  /* enable or disable low levels log */
>  void qemu_set_log(int log_flags, bool use_own_buffers)
>  {
> +    const char *fname = logfilename ?: DEFAULT_LOGFILENAME;

?: is not C99, but a GNU extension.  What is our policy on using it?
Markus Armbruster - Jan. 16, 2013, 4:06 p.m.
Eric Blake <eblake@redhat.com> writes:

> On 01/16/2013 07:36 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  qemu-log.c | 13 +++++++++----
>>  1 file changed, 9 insertions(+), 4 deletions(-)
>> 
>> diff --git a/qemu-log.c b/qemu-log.c
>> index 64a1b88..1eadf1b 100644
>> --- a/qemu-log.c
>> +++ b/qemu-log.c
>> @@ -21,10 +21,12 @@
>>  #include "qemu/log.h"
>>  
>>  #ifdef WIN32
>> -static const char *logfilename = "qemu.log";
>> +#define DEFAULT_LOGFILENAME = "qemu.log"
>
> Syntax error.

How embarrassing.  Unfortunately, it compiles...  I'll respin.

>> @@ -54,11 +56,13 @@ void qemu_log_mask(int mask, const char *fmt, ...)
>>  /* enable or disable low levels log */
>>  void qemu_set_log(int log_flags, bool use_own_buffers)
>>  {
>> +    const char *fname = logfilename ?: DEFAULT_LOGFILENAME;
>
> ?: is not C99, but a GNU extension.  What is our policy on using it?

We got several dozen instancess of ?: spread over the tree.
Blue Swirl - Jan. 17, 2013, 8:49 p.m.
On Wed, Jan 16, 2013 at 4:06 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> On 01/16/2013 07:36 AM, Markus Armbruster wrote:
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  qemu-log.c | 13 +++++++++----
>>>  1 file changed, 9 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/qemu-log.c b/qemu-log.c
>>> index 64a1b88..1eadf1b 100644
>>> --- a/qemu-log.c
>>> +++ b/qemu-log.c
>>> @@ -21,10 +21,12 @@
>>>  #include "qemu/log.h"
>>>
>>>  #ifdef WIN32
>>> -static const char *logfilename = "qemu.log";
>>> +#define DEFAULT_LOGFILENAME = "qemu.log"
>>
>> Syntax error.
>
> How embarrassing.  Unfortunately, it compiles...  I'll respin.
>
>>> @@ -54,11 +56,13 @@ void qemu_log_mask(int mask, const char *fmt, ...)
>>>  /* enable or disable low levels log */
>>>  void qemu_set_log(int log_flags, bool use_own_buffers)
>>>  {
>>> +    const char *fname = logfilename ?: DEFAULT_LOGFILENAME;
>>
>> ?: is not C99, but a GNU extension.  What is our policy on using it?
>
> We got several dozen instancess of ?: spread over the tree.

I'd not add any more. It's a bad idea to add more dependencies to
specific tools than is necessary.

Patch

diff --git a/qemu-log.c b/qemu-log.c
index 64a1b88..1eadf1b 100644
--- a/qemu-log.c
+++ b/qemu-log.c
@@ -21,10 +21,12 @@ 
 #include "qemu/log.h"
 
 #ifdef WIN32
-static const char *logfilename = "qemu.log";
+#define DEFAULT_LOGFILENAME = "qemu.log"
 #else
-static const char *logfilename = "/tmp/qemu.log";
+#define DEFAULT_LOGFILENAME "/tmp/qemu.log"
 #endif
+
+static const char *logfilename;
 FILE *qemu_logfile;
 int qemu_loglevel;
 static int log_append = 0;
@@ -54,11 +56,13 @@  void qemu_log_mask(int mask, const char *fmt, ...)
 /* enable or disable low levels log */
 void qemu_set_log(int log_flags, bool use_own_buffers)
 {
+    const char *fname = logfilename ?: DEFAULT_LOGFILENAME;
+
     qemu_loglevel = log_flags;
     if (qemu_loglevel && !qemu_logfile) {
-        qemu_logfile = fopen(logfilename, log_append ? "a" : "w");
+        qemu_logfile = fopen(fname, log_append ? "a" : "w");
         if (!qemu_logfile) {
-            perror(logfilename);
+            perror(fname);
             _exit(1);
         }
         /* must avoid mmap() usage of glibc by setting a buffer "by hand" */
@@ -84,6 +88,7 @@  void qemu_set_log(int log_flags, bool use_own_buffers)
 
 void cpu_set_log_filename(const char *filename)
 {
+    g_free((void *)logfilename);
     logfilename = g_strdup(filename);
     if (qemu_logfile) {
         fclose(qemu_logfile);