[Qemu-devel] [PATCH] Changed malloc() to g_malloc() at places where return value was not being checked

[Qemu-devel] [PATCH] Changed malloc() to g_malloc() at places where return value was not being checked

[Md Haris Iqbal]

Signed-off-by: Md Haris Iqbal <haris.phnx@gmail.com>
---
 bsd-user/elfload.c | 2 +-
 bsd-user/qemu.h    | 2 +-
 linux-user/qemu.h  | 2 +-
 thunk.c            | 2 +-
 ui/shader.c        | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
index 0a6092b..40bd1f2 100644
--- a/bsd-user/elfload.c
+++ b/bsd-user/elfload.c
@@ -1064,7 +1064,7 @@ static void load_symbols(struct elfhdr *hdr, int fd)
 
  found:
     /* Now know where the strtab and symtab are.  Snarf them. */
-    s = malloc(sizeof(*s));
+    s = g_malloc(sizeof(*s));
     syms = malloc(symtab.sh_size);
     if (!syms) {
         free(s);
diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
index 03b502a..ada4360 100644
--- a/bsd-user/qemu.h
+++ b/bsd-user/qemu.h
@@ -357,7 +357,7 @@ static inline void *lock_user(int type, abi_ulong guest_addr, long len, int copy
 #ifdef DEBUG_REMAP
     {
         void *addr;
-        addr = malloc(len);
+        addr = g_malloc(len);
         if (copy)
             memcpy(addr, g2h(guest_addr), len);
         else
diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 26b0ba2..615b9b9 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -381,7 +381,7 @@ static inline void *lock_user(int type, abi_ulong guest_addr, long len, int copy
 #ifdef DEBUG_REMAP
     {
         void *addr;
-        addr = malloc(len);
+        addr = g_malloc(len);
         if (copy)
             memcpy(addr, g2h(guest_addr), len);
         else
diff --git a/thunk.c b/thunk.c
index f057d86..bddabae 100644
--- a/thunk.c
+++ b/thunk.c
@@ -88,7 +88,7 @@ void thunk_register_struct(int id, const char *name, const argtype *types)
     for(i = 0;i < 2; i++) {
         offset = 0;
         max_align = 1;
-        se->field_offsets[i] = malloc(nb_fields * sizeof(int));
+        se->field_offsets[i] = g_malloc(nb_fields * sizeof(int));
         type_ptr = se->field_types;
         for(j = 0;j < nb_fields; j++) {
             size = thunk_type_size(type_ptr, i);
diff --git a/ui/shader.c b/ui/shader.c
index 9264009..43c6f64 100644
--- a/ui/shader.c
+++ b/ui/shader.c
@@ -83,7 +83,7 @@ GLuint qemu_gl_create_compile_shader(GLenum type, const GLchar *src)
     glGetShaderiv(shader, GL_COMPILE_STATUS, &status);
     if (!status) {
         glGetShaderiv(shader, GL_INFO_LOG_LENGTH, &length);
-        errmsg = malloc(length);
+        errmsg = g_malloc(length);
         glGetShaderInfoLog(shader, length, &length, errmsg);
         fprintf(stderr, "%s: compile %s error\n%s\n", __func__,
                 (type == GL_VERTEX_SHADER) ? "vertex" : "fragment",
-- 
1.9.1

Re: [Qemu-devel] [PATCH] Changed malloc() to g_malloc() at places where return value was not being checked

[Peter Maydell]

On 22 March 2016 at 14:33, Md Haris Iqbal <haris.phnx@gmail.com> wrote:
> Signed-off-by: Md Haris Iqbal <haris.phnx@gmail.com>

Hi. I'm afraid you can't just change malloc() calls to
g_malloc() without also changing the corresponding free()
calls to g_free().

Also, at least the thunk.c code has had patches and
discussion on-list regarding its malloc() calls. It's
often worth searching the mailing list to check you won't
be repeating work that somebody else is already doing.

> ---
>  bsd-user/elfload.c | 2 +-
>  bsd-user/qemu.h    | 2 +-
>  linux-user/qemu.h  | 2 +-
>  thunk.c            | 2 +-
>  ui/shader.c        | 2 +-
>  5 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
> index 0a6092b..40bd1f2 100644
> --- a/bsd-user/elfload.c
> +++ b/bsd-user/elfload.c
> @@ -1064,7 +1064,7 @@ static void load_symbols(struct elfhdr *hdr, int fd)
>
>   found:
>      /* Now know where the strtab and symtab are.  Snarf them. */
> -    s = malloc(sizeof(*s));
> +    s = g_malloc(sizeof(*s));
>      syms = malloc(symtab.sh_size);
>      if (!syms) {
>          free(s);

It's very odd to have only part of this data structure be
malloc and part g_malloc. We should convert all of it
or none of it.

> diff --git a/ui/shader.c b/ui/shader.c
> index 9264009..43c6f64 100644
> --- a/ui/shader.c
> +++ b/ui/shader.c
> @@ -83,7 +83,7 @@ GLuint qemu_gl_create_compile_shader(GLenum type, const GLchar *src)
>      glGetShaderiv(shader, GL_COMPILE_STATUS, &status);
>      if (!status) {
>          glGetShaderiv(shader, GL_INFO_LOG_LENGTH, &length);
> -        errmsg = malloc(length);
> +        errmsg = g_malloc(length);
>          glGetShaderInfoLog(shader, length, &length, errmsg);
>          fprintf(stderr, "%s: compile %s error\n%s\n", __func__,
>                  (type == GL_VERTEX_SHADER) ? "vertex" : "fragment",

Changes to the ui/shader.c code should be in a different
patch, since they're not related to the linux-user or
bsd-user code. (Splitting out bsd-user changes would
also be a good idea.) The idea here is that different
areas of the code have different maintainers, and so
review is by different people (and a problem in one part
of a patch doesn't then hold up an unrelated good change
in a different part).

thanks
-- PMM

Re: [Qemu-devel] [PATCH] Changed malloc() to g_malloc() at places where return value was not being checked

[haris iqbal]

On Tue, Mar 22, 2016 at 8:27 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 22 March 2016 at 14:33, Md Haris Iqbal <haris.phnx@gmail.com> wrote:
>> Signed-off-by: Md Haris Iqbal <haris.phnx@gmail.com>
>
> Hi. I'm afraid you can't just change malloc() calls to
> g_malloc() without also changing the corresponding free()
> calls to g_free().

Ok, I will do that.

>
> Also, at least the thunk.c code has had patches and
> discussion on-list regarding its malloc() calls. It's
> often worth searching the mailing list to check you won't
> be repeating work that somebody else is already doing.
>
>> ---
>>  bsd-user/elfload.c | 2 +-
>>  bsd-user/qemu.h    | 2 +-
>>  linux-user/qemu.h  | 2 +-
>>  thunk.c            | 2 +-
>>  ui/shader.c        | 2 +-
>>  5 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
>> index 0a6092b..40bd1f2 100644
>> --- a/bsd-user/elfload.c
>> +++ b/bsd-user/elfload.c
>> @@ -1064,7 +1064,7 @@ static void load_symbols(struct elfhdr *hdr, int fd)
>>
>>   found:
>>      /* Now know where the strtab and symtab are.  Snarf them. */
>> -    s = malloc(sizeof(*s));
>> +    s = g_malloc(sizeof(*s));
>>      syms = malloc(symtab.sh_size);
>>      if (!syms) {
>>          free(s);

I did that because the other one has a return value check. Which (as
discussed with Paolo Bonzini) should be done in a seperate patch.

>
> It's very odd to have only part of this data structure be
> malloc and part g_malloc. We should convert all of it
> or none of it.
>
>> diff --git a/ui/shader.c b/ui/shader.c
>> index 9264009..43c6f64 100644
>> --- a/ui/shader.c
>> +++ b/ui/shader.c
>> @@ -83,7 +83,7 @@ GLuint qemu_gl_create_compile_shader(GLenum type, const GLchar *src)
>>      glGetShaderiv(shader, GL_COMPILE_STATUS, &status);
>>      if (!status) {
>>          glGetShaderiv(shader, GL_INFO_LOG_LENGTH, &length);
>> -        errmsg = malloc(length);
>> +        errmsg = g_malloc(length);
>>          glGetShaderInfoLog(shader, length, &length, errmsg);
>>          fprintf(stderr, "%s: compile %s error\n%s\n", __func__,
>>                  (type == GL_VERTEX_SHADER) ? "vertex" : "fragment",
>
> Changes to the ui/shader.c code should be in a different
> patch, since they're not related to the linux-user or
> bsd-user code. (Splitting out bsd-user changes would
> also be a good idea.) The idea here is that different
> areas of the code have different maintainers, and so
> review is by different people (and a problem in one part
> of a patch doesn't then hold up an unrelated good change
> in a different part).

Ya, sure. I will split up the patches for different files and send
them. I can cc the seperate maintainer also then.

>
> thanks
> -- PMM



-- 

With regards,

Md Haris Iqbal,
Placement Coordinator, MTech IT
NITK Surathkal,
Contact: +91 8861996962

Re: [Qemu-devel] [PATCH] Changed malloc() to g_malloc() at places where return value was not being checked

[Paolo Bonzini]

On 22/03/2016 16:52, haris iqbal wrote:
> On Tue, Mar 22, 2016 at 8:27 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 22 March 2016 at 14:33, Md Haris Iqbal <haris.phnx@gmail.com> wrote:
>>> Signed-off-by: Md Haris Iqbal <haris.phnx@gmail.com>
>>
>> Hi. I'm afraid you can't just change malloc() calls to
>> g_malloc() without also changing the corresponding free()
>> calls to g_free().
> 
> Ok, I will do that.
> 
>>
>> Also, at least the thunk.c code has had patches and
>> discussion on-list regarding its malloc() calls. It's
>> often worth searching the mailing list to check you won't
>> be repeating work that somebody else is already doing.
>>
>>> ---
>>>  bsd-user/elfload.c | 2 +-
>>>  bsd-user/qemu.h    | 2 +-
>>>  linux-user/qemu.h  | 2 +-
>>>  thunk.c            | 2 +-
>>>  ui/shader.c        | 2 +-
>>>  5 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
>>> index 0a6092b..40bd1f2 100644
>>> --- a/bsd-user/elfload.c
>>> +++ b/bsd-user/elfload.c
>>> @@ -1064,7 +1064,7 @@ static void load_symbols(struct elfhdr *hdr, int fd)
>>>
>>>   found:
>>>      /* Now know where the strtab and symtab are.  Snarf them. */
>>> -    s = malloc(sizeof(*s));
>>> +    s = g_malloc(sizeof(*s));
>>>      syms = malloc(symtab.sh_size);
>>>      if (!syms) {
>>>          free(s);
> 
> I did that because the other one has a return value check. Which (as
> discussed with Paolo Bonzini) should be done in a seperate patch.

In this case you can convert syms to g_try_malloc.

Paolo

>>
>> It's very odd to have only part of this data structure be
>> malloc and part g_malloc. We should convert all of it
>> or none of it.
>>
>>> diff --git a/ui/shader.c b/ui/shader.c
>>> index 9264009..43c6f64 100644
>>> --- a/ui/shader.c
>>> +++ b/ui/shader.c
>>> @@ -83,7 +83,7 @@ GLuint qemu_gl_create_compile_shader(GLenum type, const GLchar *src)
>>>      glGetShaderiv(shader, GL_COMPILE_STATUS, &status);
>>>      if (!status) {
>>>          glGetShaderiv(shader, GL_INFO_LOG_LENGTH, &length);
>>> -        errmsg = malloc(length);
>>> +        errmsg = g_malloc(length);
>>>          glGetShaderInfoLog(shader, length, &length, errmsg);
>>>          fprintf(stderr, "%s: compile %s error\n%s\n", __func__,
>>>                  (type == GL_VERTEX_SHADER) ? "vertex" : "fragment",
>>
>> Changes to the ui/shader.c code should be in a different
>> patch, since they're not related to the linux-user or
>> bsd-user code. (Splitting out bsd-user changes would
>> also be a good idea.) The idea here is that different
>> areas of the code have different maintainers, and so
>> review is by different people (and a problem in one part
>> of a patch doesn't then hold up an unrelated good change
>> in a different part).
> 
> Ya, sure. I will split up the patches for different files and send
> them. I can cc the seperate maintainer also then.
> 
>>
>> thanks
>> -- PMM
> 
> 
> 

Re: [Qemu-devel] [PATCH] Changed malloc() to g_malloc() at places where return value was not being checked

[haris iqbal]

On Tue, Mar 22, 2016 at 9:35 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 22/03/2016 16:52, haris iqbal wrote:
>> On Tue, Mar 22, 2016 at 8:27 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 22 March 2016 at 14:33, Md Haris Iqbal <haris.phnx@gmail.com> wrote:
>>>> Signed-off-by: Md Haris Iqbal <haris.phnx@gmail.com>
>>>
>>> Hi. I'm afraid you can't just change malloc() calls to
>>> g_malloc() without also changing the corresponding free()
>>> calls to g_free().
>>
>> Ok, I will do that.
>>
>>>
>>> Also, at least the thunk.c code has had patches and
>>> discussion on-list regarding its malloc() calls. It's
>>> often worth searching the mailing list to check you won't
>>> be repeating work that somebody else is already doing.
>>>
>>>> ---
>>>>  bsd-user/elfload.c | 2 +-
>>>>  bsd-user/qemu.h    | 2 +-
>>>>  linux-user/qemu.h  | 2 +-
>>>>  thunk.c            | 2 +-
>>>>  ui/shader.c        | 2 +-
>>>>  5 files changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
>>>> index 0a6092b..40bd1f2 100644
>>>> --- a/bsd-user/elfload.c
>>>> +++ b/bsd-user/elfload.c
>>>> @@ -1064,7 +1064,7 @@ static void load_symbols(struct elfhdr *hdr, int fd)
>>>>
>>>>   found:
>>>>      /* Now know where the strtab and symtab are.  Snarf them. */
>>>> -    s = malloc(sizeof(*s));
>>>> +    s = g_malloc(sizeof(*s));
>>>>      syms = malloc(symtab.sh_size);
>>>>      if (!syms) {
>>>>          free(s);
>>
>> I did that because the other one has a return value check. Which (as
>> discussed with Paolo Bonzini) should be done in a seperate patch.
>
> In this case you can convert syms to g_try_malloc.

Great. I will do that.

>
> Paolo
>
>>>
>>> It's very odd to have only part of this data structure be
>>> malloc and part g_malloc. We should convert all of it
>>> or none of it.
>>>
>>>> diff --git a/ui/shader.c b/ui/shader.c
>>>> index 9264009..43c6f64 100644
>>>> --- a/ui/shader.c
>>>> +++ b/ui/shader.c
>>>> @@ -83,7 +83,7 @@ GLuint qemu_gl_create_compile_shader(GLenum type, const GLchar *src)
>>>>      glGetShaderiv(shader, GL_COMPILE_STATUS, &status);
>>>>      if (!status) {
>>>>          glGetShaderiv(shader, GL_INFO_LOG_LENGTH, &length);
>>>> -        errmsg = malloc(length);
>>>> +        errmsg = g_malloc(length);
>>>>          glGetShaderInfoLog(shader, length, &length, errmsg);
>>>>          fprintf(stderr, "%s: compile %s error\n%s\n", __func__,
>>>>                  (type == GL_VERTEX_SHADER) ? "vertex" : "fragment",
>>>
>>> Changes to the ui/shader.c code should be in a different
>>> patch, since they're not related to the linux-user or
>>> bsd-user code. (Splitting out bsd-user changes would
>>> also be a good idea.) The idea here is that different
>>> areas of the code have different maintainers, and so
>>> review is by different people (and a problem in one part
>>> of a patch doesn't then hold up an unrelated good change
>>> in a different part).
>>
>> Ya, sure. I will split up the patches for different files and send
>> them. I can cc the seperate maintainer also then.
>>
>>>
>>> thanks
>>> -- PMM
>>
>>
>>


One more question. About tracking down g_free(). I thought of
submitting for linux-user/qemu.h first. As it is done in a function
called lock_user(), which is called by many other functions (around
144, too many to be checked manually). The interesting part is, the
free is done by a pair function called unlock_user(). I just want to
ask if all those lock_user() calls has a matching unlock_user() call
to free() the malloc(), or there is a hidden free somewhere else also?
This would save a lot of time. Thanks.

(PS. I saw in the MAINTAINER file that qemu-devel@nongnu.org should be
queried for questions regarding linux-* files. So mailing here.)


-- 

With regards,

Md Haris Iqbal,
Placement Coordinator, MTech IT
NITK Surathkal,
Contact: +91 8861996962

Re: [Qemu-devel] [PATCH] Changed malloc() to g_malloc() at places where return value was not being checked

[Peter Maydell]

On 22 March 2016 at 16:15, haris iqbal <haris.phnx@gmail.com> wrote:
> One more question. About tracking down g_free(). I thought of
> submitting for linux-user/qemu.h first. As it is done in a function
> called lock_user(), which is called by many other functions (around
> 144, too many to be checked manually). The interesting part is, the
> free is done by a pair function called unlock_user(). I just want to
> ask if all those lock_user() calls has a matching unlock_user() call
> to free() the malloc(), or there is a hidden free somewhere else also?
> This would save a lot of time. Thanks.

lock_user and unlock_user should always match. (In particular
lock_user will only malloc if DEBUG_REMAP is defined, which it
is not by default, so callers can't be free()ing by mistake.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] Changed malloc() to g_malloc() at places where return value was not being checked

[haris iqbal]

On Tue, Mar 22, 2016 at 10:16 PM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 22 March 2016 at 16:15, haris iqbal <haris.phnx@gmail.com> wrote:
>> One more question. About tracking down g_free(). I thought of
>> submitting for linux-user/qemu.h first. As it is done in a function
>> called lock_user(), which is called by many other functions (around
>> 144, too many to be checked manually). The interesting part is, the
>> free is done by a pair function called unlock_user(). I just want to
>> ask if all those lock_user() calls has a matching unlock_user() call
>> to free() the malloc(), or there is a hidden free somewhere else also?
>> This would save a lot of time. Thanks.
>
> lock_user and unlock_user should always match. (In particular
> lock_user will only malloc if DEBUG_REMAP is defined, which it
> is not by default, so callers can't be free()ing by mistake.)

Thanks. Then its safe to say that changing the free() in unlock_user()
to g_free() would do the job for the malloc() to g_malloc() change in
lock_user().

>
> thanks
> -- PMM



-- 

With regards,

Md Haris Iqbal,
Placement Coordinator, MTech IT
NITK Surathkal,
Contact: +91 8861996962

Re: [Qemu-devel] [PATCH] Changed malloc() to g_malloc() at places where return value was not being checked

[Markus Armbruster]

Md Haris Iqbal <haris.phnx@gmail.com> writes:

> Signed-off-by: Md Haris Iqbal <haris.phnx@gmail.com>
> ---
>  bsd-user/elfload.c | 2 +-
>  bsd-user/qemu.h    | 2 +-
>  linux-user/qemu.h  | 2 +-
>  thunk.c            | 2 +-
>  ui/shader.c        | 2 +-
>  5 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
> index 0a6092b..40bd1f2 100644
> --- a/bsd-user/elfload.c
> +++ b/bsd-user/elfload.c
> @@ -1064,7 +1064,7 @@ static void load_symbols(struct elfhdr *hdr, int fd)
>  
>   found:
>      /* Now know where the strtab and symtab are.  Snarf them. */
> -    s = malloc(sizeof(*s));
> +    s = g_malloc(sizeof(*s));
>      syms = malloc(symtab.sh_size);
>      if (!syms) {
>          free(s);

You need to track down where s is freed and change from free() to
g_free() there.  One such spot is visible in context.  Same for the
rest of the patch.

[...]

Re: [Qemu-devel] [PATCH] Changed malloc() to g_malloc() at places where return value was not being checked

[haris iqbal]

On Tue, Mar 22, 2016 at 8:45 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Md Haris Iqbal <haris.phnx@gmail.com> writes:
>
>> Signed-off-by: Md Haris Iqbal <haris.phnx@gmail.com>
>> ---
>>  bsd-user/elfload.c | 2 +-
>>  bsd-user/qemu.h    | 2 +-
>>  linux-user/qemu.h  | 2 +-
>>  thunk.c            | 2 +-
>>  ui/shader.c        | 2 +-
>>  5 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
>> index 0a6092b..40bd1f2 100644
>> --- a/bsd-user/elfload.c
>> +++ b/bsd-user/elfload.c
>> @@ -1064,7 +1064,7 @@ static void load_symbols(struct elfhdr *hdr, int fd)
>>
>>   found:
>>      /* Now know where the strtab and symtab are.  Snarf them. */
>> -    s = malloc(sizeof(*s));
>> +    s = g_malloc(sizeof(*s));
>>      syms = malloc(symtab.sh_size);
>>      if (!syms) {
>>          free(s);
>
> You need to track down where s is freed and change from free() to
> g_free() there.  One such spot is visible in context.  Same for the
> rest of the patch.

On it. Thanks.

>
> [...]



-- 

With regards,

Md Haris Iqbal,
Placement Coordinator, MTech IT
NITK Surathkal,
Contact: +91 8861996962