On Tue, Mar 22, 2016 at 9:35 PM, Paolo Bonzini <pbonz...@redhat.com> wrote: > > > On 22/03/2016 16:52, haris iqbal wrote: >> On Tue, Mar 22, 2016 at 8:27 PM, Peter Maydell <peter.mayd...@linaro.org> >> wrote: >>> On 22 March 2016 at 14:33, Md Haris Iqbal <haris.p...@gmail.com> wrote: >>>> Signed-off-by: Md Haris Iqbal <haris.p...@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