| Message ID | 1458657219-12156-1-git-send-email-haris.phnx@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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 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
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. > > [...]
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 > > >
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.)
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
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
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",
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(-)