Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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 a...@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. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://et.redhat.com/~rjones/virt-df/
[Qemu-devel] [PATCH] Introduce QEMU_NEW()
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 a...@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); -- 1.7.5.3
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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)); -- Sasha.
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On 25 July 2011 10:32, Alexander Graf ag...@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
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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++. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On 07/25/2011 12:48 PM, Peter Maydell wrote: On 25 July 2011 10:32, Alexander Grafag...@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. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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 Grafag...@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
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On Mon, Jul 25, 2011 at 9:51 AM, Avi Kivity a...@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 a...@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
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
Am 25.07.2011 12:06, schrieb Stefan Hajnoczi: On Mon, Jul 25, 2011 at 9:51 AM, Avi Kivity a...@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 a...@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
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On Mon, Jul 25, 2011 at 11:25 AM, Kevin Wolf kw...@redhat.com wrote: Am 25.07.2011 12:06, schrieb Stefan Hajnoczi: On Mon, Jul 25, 2011 at 9:51 AM, Avi Kivity a...@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 a...@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
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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. -- mailto:av1...@comtv.ru
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
Avi Kivity a...@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. [...]
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
Kevin Wolf kw...@redhat.com writes: Am 25.07.2011 12:06, schrieb Stefan Hajnoczi: On Mon, Jul 25, 2011 at 9:51 AM, Avi Kivity a...@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 a...@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.
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On 25.07.2011, at 12:59, Markus Armbruster wrote: Avi Kivity a...@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
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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 Kivitya...@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);
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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()? -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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 Grafag...@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
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On Mon, Jul 25, 2011 at 1:09 PM, Avi Kivity a...@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.
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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é -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On Mon, Jul 25, 2011 at 3:21 PM, Anthony Liguori anth...@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.
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On 07/25/2011 09:23 AM, Blue Swirl wrote: On Mon, Jul 25, 2011 at 3:21 PM, Anthony Liguorianth...@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
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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? -- Thanks. -- Max
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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.. -- mailto:av1...@comtv.ru
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On Mon, Jul 25, 2011 at 5:51 PM, Paolo Bonzini pbonz...@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.
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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? -- mailto:av1...@comtv.ru
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
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). -- error compiling committee.c: too many arguments to function
