| Message ID | 1313689697-23627-1-git-send-email-avi@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Aug 18, 2011 at 6:48 PM, Avi Kivity <avi@redhat.com> wrote: > This makes the tracing infrastructure available to users of g_new(). > > Signed-off-by: Avi Kivity <avi@redhat.com> > --- > qemu-common.h | 1 + > qemu-malloc.c | 15 +++++++++++++++ > vl.c | 1 + > 3 files changed, 17 insertions(+), 0 deletions(-) Seems useful :) Stefan
On 18 August 2011 18:48, Avi Kivity <avi@redhat.com> wrote: > +static GMemVTable gmemvtable = { > + .malloc = qemu_malloc, > + .realloc = qemu_realloc, > + .free = qemu_free, > +}; > + > +/** > + * qemu_malloc_init: initialize memory management > + */ > +void qemu_malloc_init(void) > +{ > + g_mem_set_vtable(&gmemvtable); > +} Does this mean you can now safely allocate with g_malloc and free with qemu_free, or is mixing the two APIs like that still a no-no ? (I'm thinking about a situation where you might use a glib utility function that returned g_malloc'd memory and want to pass that back to your caller without having to either copy to qemu_malloc'd memory or require your caller to care about the distinction.) -- PMM
On 08/18/2011 09:54 PM, Peter Maydell wrote: > On 18 August 2011 18:48, Avi Kivity<avi@redhat.com> wrote: > > +static GMemVTable gmemvtable = { > > + .malloc = qemu_malloc, > > + .realloc = qemu_realloc, > > + .free = qemu_free, > > +}; > > + > > +/** > > + * qemu_malloc_init: initialize memory management > > + */ > > +void qemu_malloc_init(void) > > +{ > > + g_mem_set_vtable(&gmemvtable); > > +} > > Does this mean you can now safely allocate with g_malloc > and free with qemu_free, or is mixing the two APIs like that > still a no-no ? You can, but I'd forbid it. Mixing layers can only lead to tears later on. Best would be to convert qemu_malloc()s to g_new()s and g_malloc()s to reduce confusion. > > (I'm thinking about a situation where you might use a glib utility > function that returned g_malloc'd memory and want to pass that back > to your caller without having to either copy to qemu_malloc'd memory > or require your caller to care about the distinction.) > Changing ownership of memory is rare, I hope.
On Fri, Aug 19, 2011 at 3:22 PM, Avi Kivity <avi@redhat.com> wrote: > On 08/18/2011 09:54 PM, Peter Maydell wrote: >> >> On 18 August 2011 18:48, Avi Kivity<avi@redhat.com> wrote: >> > +static GMemVTable gmemvtable = { >> > + .malloc = qemu_malloc, >> > + .realloc = qemu_realloc, >> > + .free = qemu_free, >> > +}; >> > + >> > +/** >> > + * qemu_malloc_init: initialize memory management >> > + */ >> > +void qemu_malloc_init(void) >> > +{ >> > + g_mem_set_vtable(&gmemvtable); >> > +} >> >> Does this mean you can now safely allocate with g_malloc >> and free with qemu_free, or is mixing the two APIs like that >> still a no-no ? > > You can, but I'd forbid it. Mixing layers can only lead to tears later on. > > Best would be to convert qemu_malloc()s to g_new()s and g_malloc()s to > reduce confusion. Also plain malloc() and friends, except in tracing back end for obvious reasons. >> (I'm thinking about a situation where you might use a glib utility >> function that returned g_malloc'd memory and want to pass that back >> to your caller without having to either copy to qemu_malloc'd memory >> or require your caller to care about the distinction.) >> > > Changing ownership of memory is rare, I hope. > > -- > I have a truly marvellous patch that fixes the bug which this > signature is too narrow to contain. > >
On 08/20/2011 01:59 AM, Blue Swirl wrote: > On Fri, Aug 19, 2011 at 3:22 PM, Avi Kivity<avi@redhat.com> wrote: >> On 08/18/2011 09:54 PM, Peter Maydell wrote: >>> >>> On 18 August 2011 18:48, Avi Kivity<avi@redhat.com> wrote: >>>> +static GMemVTable gmemvtable = { >>>> + .malloc = qemu_malloc, >>>> + .realloc = qemu_realloc, >>>> + .free = qemu_free, >>>> +}; >>>> + >>>> +/** >>>> + * qemu_malloc_init: initialize memory management >>>> + */ >>>> +void qemu_malloc_init(void) >>>> +{ >>>> + g_mem_set_vtable(&gmemvtable); >>>> +} >>> >>> Does this mean you can now safely allocate with g_malloc >>> and free with qemu_free, or is mixing the two APIs like that >>> still a no-no ? >> >> You can, but I'd forbid it. Mixing layers can only lead to tears later on. >> >> Best would be to convert qemu_malloc()s to g_new()s and g_malloc()s to >> reduce confusion. > > Also plain malloc() and friends, except in tracing back end for obvious reasons. sed script: s:qemu_mallocz\( *\)(:g_malloc0\1\(:g s:qemu_malloc\( *\)(:g_malloc\1\(:g s:qemu_free\( *\)(:g_free\1\(:g s:qemu_strdup\( *\)(:g_strdup\1\(:g s:qemu_strndup\( *\)(:g_strndup\1\(:g It takes a little build magic too to make sure everything has access to glib. The patch is way too large to post. Please speak now if you have an objection otherwise I'll commit in a couple days. http://repo.or.cz/w/qemu/aliguori.git/commit/5cc99cedb46b3916dae8a565d5ffc5fb2f2e9bd6 If anyone wants to try it out first. Regards, Anthony Liguori
On Sun, Aug 21, 2011 at 3:11 AM, Anthony Liguori <anthony@codemonkey.ws> wrote: > On 08/20/2011 01:59 AM, Blue Swirl wrote: >> >> On Fri, Aug 19, 2011 at 3:22 PM, Avi Kivity<avi@redhat.com> wrote: >>> >>> On 08/18/2011 09:54 PM, Peter Maydell wrote: >>>> >>>> On 18 August 2011 18:48, Avi Kivity<avi@redhat.com> wrote: >>>>> >>>>> +static GMemVTable gmemvtable = { >>>>> + .malloc = qemu_malloc, >>>>> + .realloc = qemu_realloc, >>>>> + .free = qemu_free, >>>>> +}; >>>>> + >>>>> +/** >>>>> + * qemu_malloc_init: initialize memory management >>>>> + */ >>>>> +void qemu_malloc_init(void) >>>>> +{ >>>>> + g_mem_set_vtable(&gmemvtable); >>>>> +} >>>> >>>> Does this mean you can now safely allocate with g_malloc >>>> and free with qemu_free, or is mixing the two APIs like that >>>> still a no-no ? >>> >>> You can, but I'd forbid it. Mixing layers can only lead to tears later >>> on. >>> >>> Best would be to convert qemu_malloc()s to g_new()s and g_malloc()s to >>> reduce confusion. >> >> Also plain malloc() and friends, except in tracing back end for obvious >> reasons. > > sed script: > > s:qemu_mallocz\( *\)(:g_malloc0\1\(:g > s:qemu_malloc\( *\)(:g_malloc\1\(:g > s:qemu_free\( *\)(:g_free\1\(:g > s:qemu_strdup\( *\)(:g_strdup\1\(:g > s:qemu_strndup\( *\)(:g_strndup\1\(:g nih--; Too bad GLib does not provide a function which gives aligned memory, then also qemu_memalign() and maybe qemu_vmalloc() could be replaced. The next step (or merged with this step) should be to replace plain libc malloc/free/asprintf/strdup with g_malloc/g_free/g_strdup_printf/g_strdup. HACKING should be updated to refer to g_* versions instead of qemu_* functions. > It takes a little build magic too to make sure everything has access to > glib. > > The patch is way too large to post. Please speak now if you have an > objection otherwise I'll commit in a couple days. > > http://repo.or.cz/w/qemu/aliguori.git/commit/5cc99cedb46b3916dae8a565d5ffc5fb2f2e9bd6 > > If anyone wants to try it out first. I didn't test it but looks reasonable.
On 08/21/2011 02:17 AM, Blue Swirl wrote: > On Sun, Aug 21, 2011 at 3:11 AM, Anthony Liguori<anthony@codemonkey.ws> wrote: >> On 08/20/2011 01:59 AM, Blue Swirl wrote: >>> >>> On Fri, Aug 19, 2011 at 3:22 PM, Avi Kivity<avi@redhat.com> wrote: >>>> >>>> On 08/18/2011 09:54 PM, Peter Maydell wrote: >>>>> >>>>> On 18 August 2011 18:48, Avi Kivity<avi@redhat.com> wrote: >>>>>> >>>>>> +static GMemVTable gmemvtable = { >>>>>> + .malloc = qemu_malloc, >>>>>> + .realloc = qemu_realloc, >>>>>> + .free = qemu_free, >>>>>> +}; >>>>>> + >>>>>> +/** >>>>>> + * qemu_malloc_init: initialize memory management >>>>>> + */ >>>>>> +void qemu_malloc_init(void) >>>>>> +{ >>>>>> + g_mem_set_vtable(&gmemvtable); >>>>>> +} >>>>> >>>>> Does this mean you can now safely allocate with g_malloc >>>>> and free with qemu_free, or is mixing the two APIs like that >>>>> still a no-no ? >>>> >>>> You can, but I'd forbid it. Mixing layers can only lead to tears later >>>> on. >>>> >>>> Best would be to convert qemu_malloc()s to g_new()s and g_malloc()s to >>>> reduce confusion. >>> >>> Also plain malloc() and friends, except in tracing back end for obvious >>> reasons. >> >> sed script: >> >> s:qemu_mallocz\( *\)(:g_malloc0\1\(:g >> s:qemu_malloc\( *\)(:g_malloc\1\(:g >> s:qemu_free\( *\)(:g_free\1\(:g >> s:qemu_strdup\( *\)(:g_strdup\1\(:g >> s:qemu_strndup\( *\)(:g_strndup\1\(:g > > nih--; > > Too bad GLib does not provide a function which gives aligned memory, > then also qemu_memalign() and maybe qemu_vmalloc() could be replaced. Indeed :-/ > > The next step (or merged with this step) should be to replace plain > libc malloc/free/asprintf/strdup with > g_malloc/g_free/g_strdup_printf/g_strdup. There's not a lot of these but they need to be audited individual to make sure that the frees correspond to mallocs. > > HACKING should be updated to refer to g_* versions instead of qemu_* functions. That's included in the series. >> It takes a little build magic too to make sure everything has access to >> glib. >> >> The patch is way too large to post. Please speak now if you have an >> objection otherwise I'll commit in a couple days. >> >> http://repo.or.cz/w/qemu/aliguori.git/commit/5cc99cedb46b3916dae8a565d5ffc5fb2f2e9bd6 >> >> If anyone wants to try it out first. > > I didn't test it but looks reasonable. I've pushed so qemu_malloc is no more. Regards, Anthony Liguori
On 08/21/2011 03:24 PM, Anthony Liguori wrote: > There's not a lot of these but they need to be audited individual to > make sure that the frees correspond to mallocs. I had patches for these in the qemu_malloc world. I'll try to apply the sed script to the patches. Paolo
diff --git a/qemu-common.h b/qemu-common.h index 74d5c4b..fbe2de0 100644 --- a/qemu-common.h +++ b/qemu-common.h @@ -180,6 +180,7 @@ const char *path(const char *pathname); #define qemu_isascii(c) isascii((unsigned char)(c)) #define qemu_toascii(c) toascii((unsigned char)(c)) +void qemu_malloc_init(void); void *qemu_oom_check(void *ptr); void *qemu_malloc(size_t size); void *qemu_realloc(void *ptr, size_t size); diff --git a/qemu-malloc.c b/qemu-malloc.c index b9b3851..8b0c1ec 100644 --- a/qemu-malloc.c +++ b/qemu-malloc.c @@ -24,6 +24,21 @@ #include "qemu-common.h" #include "trace.h" #include <stdlib.h> +#include <glib.h> + +static GMemVTable gmemvtable = { + .malloc = qemu_malloc, + .realloc = qemu_realloc, + .free = qemu_free, +}; + +/** + * qemu_malloc_init: initialize memory management + */ +void qemu_malloc_init(void) +{ + g_mem_set_vtable(&gmemvtable); +} void qemu_free(void *ptr) { diff --git a/vl.c b/vl.c index c714127..7c4f8da 100644 --- a/vl.c +++ b/vl.c @@ -2106,6 +2106,7 @@ int main(int argc, char **argv, char **envp) atexit(qemu_run_exit_notifiers); error_set_progname(argv[0]); + qemu_malloc_init(); init_clocks();
This makes the tracing infrastructure available to users of g_new(). Signed-off-by: Avi Kivity <avi@redhat.com> --- qemu-common.h | 1 + qemu-malloc.c | 15 +++++++++++++++ vl.c | 1 + 3 files changed, 17 insertions(+), 0 deletions(-)