| Message ID | 1311583872-362-1-git-send-email-avi@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 25.07.2011, at 10:51, Avi Kivity wrote: > qemu_malloc() is type-unsafe as it returns a void pointer. Introduce > QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. What does this buy you over type *x = qemu_malloc(sizeof(type)); ? I find the non-C++ version easier to read even. Alex
On Mon, 2011-07-25 at 11:32 +0200, Alexander Graf wrote: > On 25.07.2011, at 10:51, Avi Kivity wrote: > > > qemu_malloc() is type-unsafe as it returns a void pointer. Introduce > > QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. > > What does this buy you over > > type *x = qemu_malloc(sizeof(type)); > > ? I find the non-C++ version easier to read even. It'll warn when you do silly things such as: struct some_struct *k; k = qemu_malloc(sizeof(k));
On 25.07.2011, at 11:37, Sasha Levin wrote: > On Mon, 2011-07-25 at 11:32 +0200, Alexander Graf wrote: >> On 25.07.2011, at 10:51, Avi Kivity wrote: >> >>> qemu_malloc() is type-unsafe as it returns a void pointer. Introduce >>> QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. >> >> What does this buy you over >> >> type *x = qemu_malloc(sizeof(type)); >> >> ? I find the non-C++ version easier to read even. > > It'll warn when you do silly things such as: > > struct some_struct *k; > > k = qemu_malloc(sizeof(k)); Hm - is there any way to get this without adding upper case C++'ish macros? Alex
On 25 July 2011 10:32, Alexander Graf <agraf@suse.de> wrote: > On 25.07.2011, at 10:51, Avi Kivity wrote: >> qemu_malloc() is type-unsafe as it returns a void pointer. Introduce >> QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. > > What does this buy you over > > type *x = qemu_malloc(sizeof(type)); > > ? I find the non-C++ version easier to read even. Yeah, while we're writing in C we should just stick to the C-like APIs, it's less confusing IMHO than wrapping it up in something else. I assume Anthony's new object model stuff will have a "create me a new foo object" API anyway, so QEMU_NEW() is possibly a bit of a namespace grab. -- PMM
On 07/25/2011 12:43 PM, Alexander Graf wrote:
> Hm - is there any way to get this without adding upper case C++'ish macros?
Switch to C++.
On 07/25/2011 12:48 PM, Peter Maydell wrote: > On 25 July 2011 10:32, Alexander Graf<agraf@suse.de> wrote: > > On 25.07.2011, at 10:51, Avi Kivity wrote: > >> qemu_malloc() is type-unsafe as it returns a void pointer. Introduce > >> QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. > > > > What does this buy you over > > > > type *x = qemu_malloc(sizeof(type)); > > > > ? I find the non-C++ version easier to read even. > > Yeah, while we're writing in C we should just stick to the C-like > APIs, it's less confusing IMHO than wrapping it up in something else. That argument can be used to block any change. You'll get used to it in time. The question is, is the new interface better or not. > I assume Anthony's new object model stuff will have a "create me a > new foo object" API anyway, so QEMU_NEW() is possibly a bit of a > namespace grab. Anthony's stuff is at a much higher level, hopefully he'll come back to the ground one day.
On 25.07.2011, at 11:52, Avi Kivity wrote: > On 07/25/2011 12:48 PM, Peter Maydell wrote: >> On 25 July 2011 10:32, Alexander Graf<agraf@suse.de> wrote: >> > On 25.07.2011, at 10:51, Avi Kivity wrote: >> >> qemu_malloc() is type-unsafe as it returns a void pointer. Introduce >> >> QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. >> > >> > What does this buy you over >> > >> > type *x = qemu_malloc(sizeof(type)); >> > >> > ? I find the non-C++ version easier to read even. >> >> Yeah, while we're writing in C we should just stick to the C-like >> APIs, it's less confusing IMHO than wrapping it up in something else. > > That argument can be used to block any change. You'll get used to it in time. The question is, is the new interface better or not. I agree that it keeps you from accidently malloc'ing a struct of pointer size. But couldn't we also just add this to checkpatch.pl? I sympathize with Peter on the rationale that keeping interfaces aligned with how C APIs usually look like helps making the code more readable. Alex
On 07/25/2011 12:56 PM, Alexander Graf wrote: > > > > That argument can be used to block any change. You'll get used to it in time. The question is, is the new interface better or not. > > I agree that it keeps you from accidently malloc'ing a struct of pointer size. But couldn't we also just add this to checkpatch.pl? Better APIs trump better patch review.
On 25.07.2011, at 12:02, Avi Kivity wrote: > On 07/25/2011 12:56 PM, Alexander Graf wrote: >> > >> > That argument can be used to block any change. You'll get used to it in time. The question is, is the new interface better or not. >> >> I agree that it keeps you from accidently malloc'ing a struct of pointer size. But couldn't we also just add this to checkpatch.pl? > > Better APIs trump better patch review. Only if you enforce them. The only sensible thing for QEMU_NEW (despite the general rule of upper case macros, I'd actually prefer this one to be lower case though since it's so often used) would be to remove qemu_malloc, declare malloc() as unusable and convert all users of qemu_malloc() to qemu_new(). Alex
On Mon, Jul 25, 2011 at 9:51 AM, Avi Kivity <avi@redhat.com> wrote: > qemu_malloc() is type-unsafe as it returns a void pointer. Introduce > QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. > > Signed-off-by: Avi Kivity <avi@redhat.com> > --- > > This is part of my memory API patchset, but doesn't really belong there. > > qemu-common.h | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/qemu-common.h b/qemu-common.h > index ba55719..66effa3 100644 > --- a/qemu-common.h > +++ b/qemu-common.h > @@ -186,6 +186,9 @@ void qemu_free(void *ptr); > char *qemu_strdup(const char *str); > char *qemu_strndup(const char *str, size_t size); > > +#define QEMU_NEW(type) ((type *)(qemu_malloc(sizeof(type)))) > +#define QEMU_NEWZ(type) ((type *)(qemu_mallocz(sizeof(type)))) Does this mean we need to duplicate the type name for each allocation? struct foo *f; ... f = qemu_malloc(sizeof(*f)); Becomes: struct foo *f; ... f = QEMU_NEW(struct foo); If you ever change the name of the type you have to search-replace these instances. The idomatic C way works well, I don't see a reason to use QEMU_NEW(). Stefan
On 07/25/2011 01:04 PM, Alexander Graf wrote: > On 25.07.2011, at 12:02, Avi Kivity wrote: > > > On 07/25/2011 12:56 PM, Alexander Graf wrote: > >> > > >> > That argument can be used to block any change. You'll get used to it in time. The question is, is the new interface better or not. > >> > >> I agree that it keeps you from accidently malloc'ing a struct of pointer size. But couldn't we also just add this to checkpatch.pl? > > > > Better APIs trump better patch review. > > Only if you enforce them. The only sensible thing for QEMU_NEW (despite the general rule of upper case macros, I'd actually prefer this one to be lower case though since it's so often used) would be to remove qemu_malloc, declare malloc() as unusable and convert all users of qemu_malloc() to qemu_new(). Some qemu_mallocs() will remain (allocating a byte array or something variable sized). I agree qemu_new() will be nicer, but that will have to wait until Blue is several light-days away from Earth.
On 07/25/2011 01:06 PM, Stefan Hajnoczi wrote: > > char *qemu_strndup(const char *str, size_t size); > > > > +#define QEMU_NEW(type) ((type *)(qemu_malloc(sizeof(type)))) > > +#define QEMU_NEWZ(type) ((type *)(qemu_mallocz(sizeof(type)))) > > Does this mean we need to duplicate the type name for each allocation? > > struct foo *f; > > ... > f = qemu_malloc(sizeof(*f)); > > Becomes: > > struct foo *f; > > ... > f = QEMU_NEW(struct foo); > > If you ever change the name of the type you have to search-replace > these instances. The idomatic C way works well, I don't see a reason > to use QEMU_NEW(). It works as long as you don't make any mistakes: f = qemu_malloc(sizeof(*g)); f = qemu_malloc(sizeof(f)); qemu_malloc() returns a void pointer, these are poisonous.
On 25.07.2011, at 12:09, Avi Kivity wrote: > On 07/25/2011 01:04 PM, Alexander Graf wrote: >> On 25.07.2011, at 12:02, Avi Kivity wrote: >> >> > On 07/25/2011 12:56 PM, Alexander Graf wrote: >> >> > >> >> > That argument can be used to block any change. You'll get used to it in time. The question is, is the new interface better or not. >> >> >> >> I agree that it keeps you from accidently malloc'ing a struct of pointer size. But couldn't we also just add this to checkpatch.pl? >> > >> > Better APIs trump better patch review. >> >> Only if you enforce them. The only sensible thing for QEMU_NEW (despite the general rule of upper case macros, I'd actually prefer this one to be lower case though since it's so often used) would be to remove qemu_malloc, declare malloc() as unusable and convert all users of qemu_malloc() to qemu_new(). > > Some qemu_mallocs() will remain (allocating a byte array or something variable sized). Right. But then we really do need a check in checkpatch.pl to see if someone's using qemu_new for simple structs. > I agree qemu_new() will be nicer, but that will have to wait until Blue is several light-days away from Earth. Blue, any disagreement on adding qemu_new() as a macro? Something used that often in upper case would seriously disturb the reading flow :) Alex
Am 25.07.2011 12:06, schrieb Stefan Hajnoczi: > On Mon, Jul 25, 2011 at 9:51 AM, Avi Kivity <avi@redhat.com> wrote: >> qemu_malloc() is type-unsafe as it returns a void pointer. Introduce >> QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. >> >> Signed-off-by: Avi Kivity <avi@redhat.com> >> --- >> >> This is part of my memory API patchset, but doesn't really belong there. >> >> qemu-common.h | 3 +++ >> 1 files changed, 3 insertions(+), 0 deletions(-) >> >> diff --git a/qemu-common.h b/qemu-common.h >> index ba55719..66effa3 100644 >> --- a/qemu-common.h >> +++ b/qemu-common.h >> @@ -186,6 +186,9 @@ void qemu_free(void *ptr); >> char *qemu_strdup(const char *str); >> char *qemu_strndup(const char *str, size_t size); >> >> +#define QEMU_NEW(type) ((type *)(qemu_malloc(sizeof(type)))) >> +#define QEMU_NEWZ(type) ((type *)(qemu_mallocz(sizeof(type)))) > > Does this mean we need to duplicate the type name for each allocation? > > struct foo *f; > > ... > f = qemu_malloc(sizeof(*f)); > > Becomes: > > struct foo *f; > > ... > f = QEMU_NEW(struct foo); Maybe we should allow this and make it the usual pattern: f = qemu_new(typeof(*f)); It's gcc specific, but we already don't care about portability to other compilers in more places. On the other hand, how many bugs did we have recently that were caused by a wrong sizeof for qemu_malloc? As far as I can say, there's no real reason to do it. I think it's the same kind of discussion as with forbidding qemu_malloc(0) (except that this time it just won't improve things much instead of being really stupid). Kevin
On Mon, Jul 25, 2011 at 11:25 AM, Kevin Wolf <kwolf@redhat.com> wrote: > Am 25.07.2011 12:06, schrieb Stefan Hajnoczi: >> On Mon, Jul 25, 2011 at 9:51 AM, Avi Kivity <avi@redhat.com> wrote: >>> qemu_malloc() is type-unsafe as it returns a void pointer. Introduce >>> QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. >>> >>> Signed-off-by: Avi Kivity <avi@redhat.com> >>> --- >>> >>> This is part of my memory API patchset, but doesn't really belong there. >>> >>> qemu-common.h | 3 +++ >>> 1 files changed, 3 insertions(+), 0 deletions(-) >>> >>> diff --git a/qemu-common.h b/qemu-common.h >>> index ba55719..66effa3 100644 >>> --- a/qemu-common.h >>> +++ b/qemu-common.h >>> @@ -186,6 +186,9 @@ void qemu_free(void *ptr); >>> char *qemu_strdup(const char *str); >>> char *qemu_strndup(const char *str, size_t size); >>> >>> +#define QEMU_NEW(type) ((type *)(qemu_malloc(sizeof(type)))) >>> +#define QEMU_NEWZ(type) ((type *)(qemu_mallocz(sizeof(type)))) >> >> Does this mean we need to duplicate the type name for each allocation? >> >> struct foo *f; >> >> ... >> f = qemu_malloc(sizeof(*f)); >> >> Becomes: >> >> struct foo *f; >> >> ... >> f = QEMU_NEW(struct foo); > > Maybe we should allow this and make it the usual pattern: > > f = qemu_new(typeof(*f)); > > It's gcc specific, but we already don't care about portability to other > compilers in more places. > > On the other hand, how many bugs did we have recently that were caused > by a wrong sizeof for qemu_malloc? As far as I can say, there's no real > reason to do it. I think it's the same kind of discussion as with > forbidding qemu_malloc(0) (except that this time it just won't improve > things much instead of being really stupid). Totally agree. In theory you can add stuff on top to prevent known bad uses but in practice it's not worth obfuscating the code. Stefan
On Mon, 25 Jul 2011, Alexander Graf wrote: > > On 25.07.2011, at 12:09, Avi Kivity wrote: > > > On 07/25/2011 01:04 PM, Alexander Graf wrote: > >> On 25.07.2011, at 12:02, Avi Kivity wrote: > >> > >> > On 07/25/2011 12:56 PM, Alexander Graf wrote: > >> >> > > >> >> > That argument can be used to block any change. You'll get used to it in time. The question is, is the new interface better or not. > >> >> > >> >> I agree that it keeps you from accidently malloc'ing a struct of pointer size. But couldn't we also just add this to checkpatch.pl? > >> > > >> > Better APIs trump better patch review. > >> > >> Only if you enforce them. The only sensible thing for QEMU_NEW (despite the general rule of upper case macros, I'd actually prefer this one to be lower case though since it's so often used) would be to remove qemu_malloc, declare malloc() as unusable and convert all users of qemu_malloc() to qemu_new(). > > > > Some qemu_mallocs() will remain (allocating a byte array or something variable sized). > > Right. But then we really do need a check in checkpatch.pl to see if someone's using qemu_new for simple structs. > > > I agree qemu_new() will be nicer, but that will have to wait until Blue is several light-days away from Earth. > > Blue, any disagreement on adding qemu_new() as a macro? Something used > that often in upper case would seriously disturb the reading flow :) So do not use it then, macros should be uppercase.
Avi Kivity <avi@redhat.com> writes: > On 07/25/2011 01:04 PM, Alexander Graf wrote: >> On 25.07.2011, at 12:02, Avi Kivity wrote: >> >> > On 07/25/2011 12:56 PM, Alexander Graf wrote: >> >> > >> >> > That argument can be used to block any change. You'll get used to it in time. The question is, is the new interface better or not. >> >> >> >> I agree that it keeps you from accidently malloc'ing a struct of pointer size. But couldn't we also just add this to checkpatch.pl? >> > >> > Better APIs trump better patch review. >> >> Only if you enforce them. The only sensible thing for QEMU_NEW (despite the general rule of upper case macros, I'd actually prefer this one to be lower case though since it's so often used) would be to remove qemu_malloc, declare malloc() as unusable and convert all users of qemu_malloc() to qemu_new(). > > Some qemu_mallocs() will remain (allocating a byte array or something > variable sized). Byte array: add the obvious type-safe allocator for a variable-sized array T[N], then use it with unsigned char for T. In fact, I find QEMU_NEW() pretty pointless without a buddy for arrays. Still not covered: allocating a struct with a variable-size array as final member. I guess a solution for that can be found if we care enough. [...]
Kevin Wolf <kwolf@redhat.com> writes: > Am 25.07.2011 12:06, schrieb Stefan Hajnoczi: >> On Mon, Jul 25, 2011 at 9:51 AM, Avi Kivity <avi@redhat.com> wrote: >>> qemu_malloc() is type-unsafe as it returns a void pointer. Introduce >>> QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. >>> >>> Signed-off-by: Avi Kivity <avi@redhat.com> >>> --- >>> >>> This is part of my memory API patchset, but doesn't really belong there. >>> >>> qemu-common.h | 3 +++ >>> 1 files changed, 3 insertions(+), 0 deletions(-) >>> >>> diff --git a/qemu-common.h b/qemu-common.h >>> index ba55719..66effa3 100644 >>> --- a/qemu-common.h >>> +++ b/qemu-common.h >>> @@ -186,6 +186,9 @@ void qemu_free(void *ptr); >>> char *qemu_strdup(const char *str); >>> char *qemu_strndup(const char *str, size_t size); >>> >>> +#define QEMU_NEW(type) ((type *)(qemu_malloc(sizeof(type)))) >>> +#define QEMU_NEWZ(type) ((type *)(qemu_mallocz(sizeof(type)))) >> >> Does this mean we need to duplicate the type name for each allocation? >> >> struct foo *f; >> >> ... >> f = qemu_malloc(sizeof(*f)); >> >> Becomes: >> >> struct foo *f; >> >> ... >> f = QEMU_NEW(struct foo); > > Maybe we should allow this and make it the usual pattern: > > f = qemu_new(typeof(*f)); > > It's gcc specific, but we already don't care about portability to other > compilers in more places. > > On the other hand, how many bugs did we have recently that were caused > by a wrong sizeof for qemu_malloc? As far as I can say, there's no real > reason to do it. I think it's the same kind of discussion as with > forbidding qemu_malloc(0) (except that this time it just won't improve > things much instead of being really stupid). Side-stepping the stupid "OMG malloc(0) is weird, therefore we must make qemu_malloc(0) differently weird" controversy would be useful all by itself.
On 25.07.2011, at 12:59, Markus Armbruster wrote: > Avi Kivity <avi@redhat.com> writes: > >> On 07/25/2011 01:04 PM, Alexander Graf wrote: >>> On 25.07.2011, at 12:02, Avi Kivity wrote: >>> >>>> On 07/25/2011 12:56 PM, Alexander Graf wrote: >>>>>> >>>>>> That argument can be used to block any change. You'll get used to it in time. The question is, is the new interface better or not. >>>>> >>>>> I agree that it keeps you from accidently malloc'ing a struct of pointer size. But couldn't we also just add this to checkpatch.pl? >>>> >>>> Better APIs trump better patch review. >>> >>> Only if you enforce them. The only sensible thing for QEMU_NEW (despite the general rule of upper case macros, I'd actually prefer this one to be lower case though since it's so often used) would be to remove qemu_malloc, declare malloc() as unusable and convert all users of qemu_malloc() to qemu_new(). >> >> Some qemu_mallocs() will remain (allocating a byte array or something >> variable sized). > > Byte array: add the obvious type-safe allocator for a variable-sized > array T[N], then use it with unsigned char for T. > > In fact, I find QEMU_NEW() pretty pointless without a buddy for arrays. #define QEMU_NEW_MULTI(type, len) ((type *)(qemu_mallocz(sizeof(type) * len))) char *arr = QEMU_NEW_MULTI(char, 1024); > Still not covered: allocating a struct with a variable-size array as > final member. I guess a solution for that can be found if we care > enough. Yeah, but at the end of the day I'd assume most of us know C and can just open code this all, no? Alex
On 07/25/2011 12:32 PM, Alexander Graf wrote: > On 25.07.2011, at 10:51, Avi Kivity wrote: > > > qemu_malloc() is type-unsafe as it returns a void pointer. Introduce > > QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. > > What does this buy you over > > type *x = qemu_malloc(sizeof(type)); > > ? I find the non-C++ version easier to read even. > I'm using it as MemoryRegion *phys_flash = QEMU_NEW(MemoryRegion); instead of MemoryRegion *phys_flash = qemu_malloc(sizeof(*phys_flash)); I find it shorter, and if I make a mistake, the compiler shouts at me instead of a runtime crash.
On 07/25/2011 02:02 PM, Markus Armbruster wrote: > Side-stepping the stupid "OMG malloc(0) is weird, therefore we must make > qemu_malloc(0) differently weird" controversy would be useful all by > itself. If we all work together, we can make this thread even bigger than the tools/kvm pull request.
On 07/25/2011 03:51 AM, Avi Kivity wrote: > qemu_malloc() is type-unsafe as it returns a void pointer. Introduce > QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. Just use g_new() and g_new0() Regards, Anthony Liguori > > Signed-off-by: Avi Kivity<avi@redhat.com> > --- > > This is part of my memory API patchset, but doesn't really belong there. > > qemu-common.h | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/qemu-common.h b/qemu-common.h > index ba55719..66effa3 100644 > --- a/qemu-common.h > +++ b/qemu-common.h > @@ -186,6 +186,9 @@ void qemu_free(void *ptr); > char *qemu_strdup(const char *str); > char *qemu_strndup(const char *str, size_t size); > > +#define QEMU_NEW(type) ((type *)(qemu_malloc(sizeof(type)))) > +#define QEMU_NEWZ(type) ((type *)(qemu_mallocz(sizeof(type)))) > + > void qemu_mutex_lock_iothread(void); > void qemu_mutex_unlock_iothread(void); >
On 07/25/2011 03:11 PM, Anthony Liguori wrote: > On 07/25/2011 03:51 AM, Avi Kivity wrote: >> qemu_malloc() is type-unsafe as it returns a void pointer. Introduce >> QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. > > Just use g_new() and g_new0() > These bypass qemu_malloc(). Are we okay with that? I suppose so, since many library functions can allocate memory and bypass qemu_malloc()?
On 07/25/2011 06:11 AM, Alexander Graf wrote: > > #define QEMU_NEW_MULTI(type, len) ((type *)(qemu_mallocz(sizeof(type) * len))) > > char *arr = QEMU_NEW_MULTI(char, 1024); > >> Still not covered: allocating a struct with a variable-size array as >> final member. I guess a solution for that can be found if we care >> enough. > > Yeah, but at the end of the day I'd assume most of us know C and can just open code this all, no? While it's always fun to reinvent things, glib has already solved all of this and we're already dependent on it in the build: http://developer.gnome.org/glib/stable/glib-Memory-Allocation.html It also has fancy ways to hook memory allocation for debugging. Regards, Anthony Liguori > > Alex > >
On 07/25/2011 07:18 AM, Avi Kivity wrote: > On 07/25/2011 03:11 PM, Anthony Liguori wrote: >> On 07/25/2011 03:51 AM, Avi Kivity wrote: >>> qemu_malloc() is type-unsafe as it returns a void pointer. Introduce >>> QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. >> >> Just use g_new() and g_new0() >> > > These bypass qemu_malloc(). Are we okay with that? Yes. We can just make qemu_malloc use g_malloc. > I suppose so, since many library functions can allocate memory and > bypass qemu_malloc()? Right. Regards, Anthony Liguori >
On 07/25/2011 04:52 AM, Avi Kivity wrote: > On 07/25/2011 12:48 PM, Peter Maydell wrote: >> On 25 July 2011 10:32, Alexander Graf<agraf@suse.de> wrote: >> > On 25.07.2011, at 10:51, Avi Kivity wrote: >> >> qemu_malloc() is type-unsafe as it returns a void pointer. Introduce >> >> QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. >> > >> > What does this buy you over >> > >> > type *x = qemu_malloc(sizeof(type)); >> > >> > ? I find the non-C++ version easier to read even. >> >> Yeah, while we're writing in C we should just stick to the C-like >> APIs, it's less confusing IMHO than wrapping it up in something else. > > That argument can be used to block any change. You'll get used to it in > time. The question is, is the new interface better or not. > >> I assume Anthony's new object model stuff will have a "create me a >> new foo object" API anyway, so QEMU_NEW() is possibly a bit of a >> namespace grab. > > Anthony's stuff is at a much higher level, hopefully he'll come back to > the ground one day. The point of introducing glib was to address things like this. We need to start making heavier use of what it provides. Regards, Anthony Liguori
On 07/25/2011 03:21 PM, Anthony Liguori wrote: > On 07/25/2011 07:18 AM, Avi Kivity wrote: >> On 07/25/2011 03:11 PM, Anthony Liguori wrote: >>> On 07/25/2011 03:51 AM, Avi Kivity wrote: >>>> qemu_malloc() is type-unsafe as it returns a void pointer. Introduce >>>> QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. >>> >>> Just use g_new() and g_new0() >>> >> >> These bypass qemu_malloc(). Are we okay with that? > > Yes. We can just make qemu_malloc use g_malloc. > Excellent. Patch withdrawn.
On Mon, Jul 25, 2011 at 1:09 PM, Avi Kivity <avi@redhat.com> wrote: > On 07/25/2011 01:04 PM, Alexander Graf wrote: >> >> On 25.07.2011, at 12:02, Avi Kivity wrote: >> >> > On 07/25/2011 12:56 PM, Alexander Graf wrote: >> >> > >> >> > That argument can be used to block any change. You'll get used to >> >> it in time. The question is, is the new interface better or not. >> >> >> >> I agree that it keeps you from accidently malloc'ing a struct of >> >> pointer size. But couldn't we also just add this to checkpatch.pl? >> > >> > Better APIs trump better patch review. >> >> Only if you enforce them. The only sensible thing for QEMU_NEW (despite >> the general rule of upper case macros, I'd actually prefer this one to be >> lower case though since it's so often used) would be to remove qemu_malloc, >> declare malloc() as unusable and convert all users of qemu_malloc() to >> qemu_new(). > > Some qemu_mallocs() will remain (allocating a byte array or something > variable sized). > > I agree qemu_new() will be nicer, but that will have to wait until Blue is > several light-days away from Earth. There is no escape. Don't make me destroy you. You cannot hide forever, Luke.
On 07/25/2011 05:16 PM, Blue Swirl wrote:
> There is no escape. Don't make me destroy you. You cannot hide forever, Luke.
Touché
On Mon, Jul 25, 2011 at 3:21 PM, Anthony Liguori <anthony@codemonkey.ws> wrote: > On 07/25/2011 07:18 AM, Avi Kivity wrote: >> >> On 07/25/2011 03:11 PM, Anthony Liguori wrote: >>> >>> On 07/25/2011 03:51 AM, Avi Kivity wrote: >>>> >>>> qemu_malloc() is type-unsafe as it returns a void pointer. Introduce >>>> QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. >>> >>> Just use g_new() and g_new0() >>> >> >> These bypass qemu_malloc(). Are we okay with that? > > Yes. We can just make qemu_malloc use g_malloc. It would be also possible to make g_malloc() use qemu_malloc(). That way we could keep the tracepoints which would lose their value with g_malloc() otherwise.
On 07/25/2011 09:23 AM, Blue Swirl wrote: > On Mon, Jul 25, 2011 at 3:21 PM, Anthony Liguori<anthony@codemonkey.ws> wrote: >> On 07/25/2011 07:18 AM, Avi Kivity wrote: >>> >>> On 07/25/2011 03:11 PM, Anthony Liguori wrote: >>>> >>>> On 07/25/2011 03:51 AM, Avi Kivity wrote: >>>>> >>>>> qemu_malloc() is type-unsafe as it returns a void pointer. Introduce >>>>> QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. >>>> >>>> Just use g_new() and g_new0() >>>> >>> >>> These bypass qemu_malloc(). Are we okay with that? >> >> Yes. We can just make qemu_malloc use g_malloc. > > It would be also possible to make g_malloc() use qemu_malloc(). That > way we could keep the tracepoints which would lose their value with > g_malloc() otherwise. Or just add tracepoints to g_malloc()... But yeah, the point is, we ought to unify to a standard library function instead of inventing our own version of everything. Regards, Anthony Liguori >
>>>>>> qemu_malloc() is type-unsafe as it returns a void pointer. Introduce >>>>>> QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. >>>>> >>>>> Just use g_new() and g_new0() >>>>> >>>> >>>> These bypass qemu_malloc(). Are we okay with that? >>> >>> Yes. We can just make qemu_malloc use g_malloc. >> >> It would be also possible to make g_malloc() use qemu_malloc(). That >> way we could keep the tracepoints which would lose their value with >> g_malloc() otherwise. > > Or just add tracepoints to g_malloc()... > > But yeah, the point is, we ought to unify to a standard library function > instead of inventing our own version of everything. What about zero-size allocations for which g_malloc would return NULL?
On 07/25/2011 09:30 AM, Max Filippov wrote: >>>>>>> qemu_malloc() is type-unsafe as it returns a void pointer. Introduce >>>>>>> QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. >>>>>> >>>>>> Just use g_new() and g_new0() >>>>>> >>>>> >>>>> These bypass qemu_malloc(). Are we okay with that? >>>> >>>> Yes. We can just make qemu_malloc use g_malloc. >>> >>> It would be also possible to make g_malloc() use qemu_malloc(). That >>> way we could keep the tracepoints which would lose their value with >>> g_malloc() otherwise. >> >> Or just add tracepoints to g_malloc()... >> >> But yeah, the point is, we ought to unify to a standard library function >> instead of inventing our own version of everything. > > What about zero-size allocations for which g_malloc would return NULL? Using a standard, well documented, rich interface trumps any arguments about the semantics of zero-sized allocation. Regards, Anthony Liguori
On Mon, 25 Jul 2011, Anthony Liguori wrote: > On 07/25/2011 09:30 AM, Max Filippov wrote: > > > > > > > > qemu_malloc() is type-unsafe as it returns a void pointer. > > > > > > > > Introduce > > > > > > > > QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. > > > > > > > > > > > > > > Just use g_new() and g_new0() > > > > > > > > > > > > > > > > > > > These bypass qemu_malloc(). Are we okay with that? > > > > > > > > > > Yes. We can just make qemu_malloc use g_malloc. > > > > > > > > It would be also possible to make g_malloc() use qemu_malloc(). That > > > > way we could keep the tracepoints which would lose their value with > > > > g_malloc() otherwise. > > > > > > Or just add tracepoints to g_malloc()... > > > > > > But yeah, the point is, we ought to unify to a standard library function > > > instead of inventing our own version of everything. > > > > What about zero-size allocations for which g_malloc would return NULL? > > Using a standard, well documented, rich interface trumps any arguments about > the semantics of zero-sized allocation. Right right.. only g_new aborts on zero..
On 07/25/2011 05:47 PM, malc wrote: > Right right.. only g_new aborts on zero.. > "If n_structs is 0 it returns NULL <http://developer.gnome.org/glib/2.28/glib-Standard-Macros.html#NULL:CAPS>. " It's annoying that it takes this parameter at all, but I can live with it.
On 07/25/2011 04:23 PM, Blue Swirl wrote: > > Yes. We can just make qemu_malloc use g_malloc. > > It would be also possible to make g_malloc() use qemu_malloc(). That > way we could keep the tracepoints which would lose their value with > g_malloc() otherwise. qemu_malloc uses g_malloc => you keep tracepoints, you just do not trace memory allocated by glib g_malloc uses qemu_malloc => you keep and expand tracepoints, you lose the very nicely tuned allocator The former is much less code, however it requires qemu_malloc to be always balanced with qemu_free (patches ready and on my github tree, won't be sent before KVM Forum though...). Paolo
On Mon, Jul 25, 2011 at 5:51 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 07/25/2011 04:23 PM, Blue Swirl wrote: >> >> > Yes. We can just make qemu_malloc use g_malloc. >> >> It would be also possible to make g_malloc() use qemu_malloc(). That >> way we could keep the tracepoints which would lose their value with >> g_malloc() otherwise. > > qemu_malloc uses g_malloc => you keep tracepoints, you just do not trace > memory allocated by glib Unless the plan is to replace all qemu_malloc() calls with calls to g_malloc(). > g_malloc uses qemu_malloc => you keep and expand tracepoints, you lose the > very nicely tuned allocator It is replaced by libc malloc() which shouldn't be so bad either. > The former is much less code, however it requires qemu_malloc to be always > balanced with qemu_free (patches ready and on my github tree, won't be sent > before KVM Forum though...). Freeing qemu_malloc() memory with plain free() is a bug.
On Mon, 25 Jul 2011, Avi Kivity wrote: > On 07/25/2011 05:47 PM, malc wrote: > > Right right.. only g_new aborts on zero.. > > > > "If n_structs is 0 it returns NULL > <http://developer.gnome.org/glib/2.28/glib-Standard-Macros.html#NULL:CAPS>. " Right you are. > > It's annoying that it takes this parameter at all, but I can live with it. > n_structs?
On 07/25/2011 05:58 PM, malc wrote: > > > > It's annoying that it takes this parameter at all, but I can live with it. > > > > n_structs? Yes. It's 1 in 1-epsilon of all cases. Would have preferred g_new and G_new_array instead.
On 07/25/11 12:06, Stefan Hajnoczi wrote: >> +#define QEMU_NEW(type) ((type *)(qemu_malloc(sizeof(type)))) >> > +#define QEMU_NEWZ(type) ((type *)(qemu_mallocz(sizeof(type)))) > Does this mean we need to duplicate the type name for each allocation? > > struct foo *f; > > ... > f = qemu_malloc(sizeof(*f)); > > Becomes: > > struct foo *f; > > ... > f = QEMU_NEW(struct foo); > > If you ever change the name of the type you have to search-replace > these instances. The idomatic C way works well, I don't see a reason > to use QEMU_NEW(). You're right, and it will promote even more abuse of the ugly typedefs. This really makes the code less readable, especially for outsiders :( Jes
On 07/25/2011 10:10 AM, Jes Sorensen wrote: > On 07/25/11 12:06, Stefan Hajnoczi wrote: >>> +#define QEMU_NEW(type) ((type *)(qemu_malloc(sizeof(type)))) >>>> +#define QEMU_NEWZ(type) ((type *)(qemu_mallocz(sizeof(type)))) >> Does this mean we need to duplicate the type name for each allocation? >> >> struct foo *f; >> >> ... >> f = qemu_malloc(sizeof(*f)); >> >> Becomes: >> >> struct foo *f; >> >> ... >> f = QEMU_NEW(struct foo); >> >> If you ever change the name of the type you have to search-replace >> these instances. The idomatic C way works well, I don't see a reason >> to use QEMU_NEW(). > > You're right, and it will promote even more abuse of the ugly typedefs. > This really makes the code less readable, especially for outsiders :( I don't think it really matters either way. If some people prefer to use g_new(struct foo, 1) vs. g_malloc(sizeof(*f)), I don't think it significantly impacts overall code readability. But having nice, documentation for key internal APIs does which is why using the glib interfaces makes sense IMHO. Regards, Anthony Liguori > > Jes > > >
On 07/25/11 17:15, Anthony Liguori wrote: > On 07/25/2011 10:10 AM, Jes Sorensen wrote: >> On 07/25/11 12:06, Stefan Hajnoczi wrote: >>>> +#define QEMU_NEW(type) ((type *)(qemu_malloc(sizeof(type)))) >>>>> +#define QEMU_NEWZ(type) ((type *)(qemu_mallocz(sizeof(type)))) >>> Does this mean we need to duplicate the type name for each allocation? >>> >>> struct foo *f; >>> >>> ... >>> f = qemu_malloc(sizeof(*f)); >>> >>> Becomes: >>> >>> struct foo *f; >>> >>> ... >>> f = QEMU_NEW(struct foo); >>> >>> If you ever change the name of the type you have to search-replace >>> these instances. The idomatic C way works well, I don't see a reason >>> to use QEMU_NEW(). >> >> You're right, and it will promote even more abuse of the ugly typedefs. >> This really makes the code less readable, especially for outsiders :( > > I don't think it really matters either way. If some people prefer to > use g_new(struct foo, 1) vs. g_malloc(sizeof(*f)), I don't think it > significantly impacts overall code readability. > > But having nice, documentation for key internal APIs does which is why > using the glib interfaces makes sense IMHO. Using the commands consistently does have an impact, and at least with qemu_malloc() it is obvious what they are and how they behave. The proposed macros on the other hand requires everybody to go look up the macro to find out what it is trying to do. Jes
On 07/25/2011 06:17 PM, Jes Sorensen wrote: > Using the commands consistently does have an impact, and at least with > qemu_malloc() it is obvious what they are and how they behave. The > proposed macros on the other hand requires everybody to go look up the > macro to find out what it is trying to do. That's true for every single function and macro that qemu defines.
On 07/25/11 17:20, Avi Kivity wrote: > On 07/25/2011 06:17 PM, Jes Sorensen wrote: >> Using the commands consistently does have an impact, and at least with >> qemu_malloc() it is obvious what they are and how they behave. The >> proposed macros on the other hand requires everybody to go look up the >> macro to find out what it is trying to do. > > That's true for every single function and macro that qemu defines. > Of course, and every time it adds complexity for reading it. In this particular case it seems to simply make the code worse for no gain. Jes
On 07/25/2011 04:56 PM, Blue Swirl wrote: > > qemu_malloc uses g_malloc => you keep tracepoints, you just do not trace > > memory allocated by glib > > Unless the plan is to replace all qemu_malloc() calls with calls to g_malloc(). We can worry when the day comes... there is already another task blocking that replacement (balancing qemu_malloc with qemu_free). > > The former is much less code, however it requires qemu_malloc to be always > > balanced with qemu_free (patches ready and on my github tree, won't be sent > > before KVM Forum though...). > > Freeing qemu_malloc() memory with plain free() is a bug. We have many bugs, then. :) Paolo
On 07/25/2011 06:21 PM, Jes Sorensen wrote: > On 07/25/11 17:20, Avi Kivity wrote: > > On 07/25/2011 06:17 PM, Jes Sorensen wrote: > >> Using the commands consistently does have an impact, and at least with > >> qemu_malloc() it is obvious what they are and how they behave. The > >> proposed macros on the other hand requires everybody to go look up the > >> macro to find out what it is trying to do. > > > > That's true for every single function and macro that qemu defines. > > > > Of course, and every time it adds complexity for reading it. In this > particular case it seems to simply make the code worse for no gain. I guess type safety doesn't matter to you.
On 07/25/11 17:24, Avi Kivity wrote: > On 07/25/2011 06:21 PM, Jes Sorensen wrote: >> On 07/25/11 17:20, Avi Kivity wrote: >> > On 07/25/2011 06:17 PM, Jes Sorensen wrote: >> >> Using the commands consistently does have an impact, and at least >> with >> >> qemu_malloc() it is obvious what they are and how they behave. The >> >> proposed macros on the other hand requires everybody to go look up >> the >> >> macro to find out what it is trying to do. >> > >> > That's true for every single function and macro that qemu defines. >> > >> >> Of course, and every time it adds complexity for reading it. In this >> particular case it seems to simply make the code worse for no gain. > > I guess type safety doesn't matter to you. In my experience it's one of the lesser problems in the code. Jes
On 07/25/2011 06:28 PM, Jes Sorensen wrote: > > > > I guess type safety doesn't matter to you. > > In my experience it's one of the lesser problems in the code. > It's a big issue to someone making widespread changes in the code (like me now).
On Mon, Jul 25, 2011 at 11:51:12AM +0300, Avi Kivity wrote: > qemu_malloc() is type-unsafe as it returns a void pointer. Introduce > QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. > > Signed-off-by: Avi Kivity <avi@redhat.com> > --- > > This is part of my memory API patchset, but doesn't really belong there. > > qemu-common.h | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/qemu-common.h b/qemu-common.h > index ba55719..66effa3 100644 > --- a/qemu-common.h > +++ b/qemu-common.h > @@ -186,6 +186,9 @@ void qemu_free(void *ptr); > char *qemu_strdup(const char *str); > char *qemu_strndup(const char *str, size_t size); > > +#define QEMU_NEW(type) ((type *)(qemu_malloc(sizeof(type)))) > +#define QEMU_NEWZ(type) ((type *)(qemu_mallocz(sizeof(type)))) > + > void qemu_mutex_lock_iothread(void); > void qemu_mutex_unlock_iothread(void); FYI libvirt have been doing something similar, perhaps even more far-reaching: http://libvirt.org/hacking.html#memalloc http://libvirt.org/git/?p=libvirt.git;a=blob;f=src/util/memory.h;hb=HEAD The libvirt versions are designed to catch errors in situations such as: - trying to allocate zero-sized objects when the underlying malloc returns NULL for zero-sized objects - trying to allocate N * M-sized objects when N * M overflows - realloc fails, don't forget the original pointer Rich.
diff --git a/qemu-common.h b/qemu-common.h index ba55719..66effa3 100644 --- a/qemu-common.h +++ b/qemu-common.h @@ -186,6 +186,9 @@ void qemu_free(void *ptr); char *qemu_strdup(const char *str); char *qemu_strndup(const char *str, size_t size); +#define QEMU_NEW(type) ((type *)(qemu_malloc(sizeof(type)))) +#define QEMU_NEWZ(type) ((type *)(qemu_mallocz(sizeof(type)))) + void qemu_mutex_lock_iothread(void); void qemu_mutex_unlock_iothread(void);
qemu_malloc() is type-unsafe as it returns a void pointer. Introduce QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. Signed-off-by: Avi Kivity <avi@redhat.com> --- This is part of my memory API patchset, but doesn't really belong there. qemu-common.h | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)