Re: [Qemu-trivial] [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 <address@hidden> wrote:
> On 22 March 2016 at 14:33, Md Haris Iqbal <address@hidden> wrote:
>> Signed-off-by: Md Haris Iqbal <address@hidden>
>
> 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