[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] Changed malloc() to g_malloc() at places where
From: |
haris iqbal |
Subject: |
Re: [Qemu-devel] [PATCH] Changed malloc() to g_malloc() at places where return value was not being checked |
Date: |
Tue, 22 Mar 2016 21:45:54 +0530 |
On Tue, Mar 22, 2016 at 9:35 PM, Paolo Bonzini <address@hidden> wrote:
>
>
> On 22/03/2016 16:52, haris iqbal wrote:
>> 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.
>
> 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 address@hidden 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, Markus Armbruster, 2016/03/22