[PATCH] Introduce QEMU_NEW()

[PATCH] Introduce QEMU_NEW()

[Avi Kivity]

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);
 
-- 
1.7.5.3

[Qemu-devel] [PATCH] Introduce QEMU_NEW()

[Avi Kivity]

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);
 
-- 
1.7.5.3

Re: [PATCH] Introduce QEMU_NEW()

[Alexander Graf]

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()

[Alexander Graf]

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: [PATCH] Introduce QEMU_NEW()

[Sasha Levin]

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()

[Sasha Levin]

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: [PATCH] Introduce QEMU_NEW()

[Alexander Graf]

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()

[Alexander Graf]

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()

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[Peter Maydell]

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

Re: [PATCH] Introduce QEMU_NEW()

[Avi Kivity]

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()

[Avi Kivity]

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()

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] Introduce QEMU_NEW()

[Alexander Graf]

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

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[Alexander Graf]

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

Re: [PATCH] Introduce QEMU_NEW()

[Avi Kivity]

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()

[Avi Kivity]

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()

[Alexander Graf]

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: [PATCH] Introduce QEMU_NEW()

[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);

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()

[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);

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()

[Avi Kivity]

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()

[Avi Kivity]

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: [PATCH] Introduce QEMU_NEW()

[Alexander Graf]

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()

[Alexander Graf]

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()

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[Stefan Hajnoczi]

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

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[Stefan Hajnoczi]

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

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[malc]

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:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[malc]

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:av1474@comtv.ru

Re: [PATCH] Introduce QEMU_NEW()

[Markus Armbruster]

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.

[...]

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[Markus Armbruster]

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.

[...]

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[Alexander Graf]

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

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[Alexander Graf]

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

Re: [PATCH] Introduce QEMU_NEW()

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] Introduce QEMU_NEW()

[Anthony Liguori]

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);
>

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[Anthony Liguori]

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);
>

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[Avi Kivity]

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()

[Anthony Liguori]

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()

[Anthony Liguori]

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()

[Anthony Liguori]

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()

[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

Re: [PATCH] Introduce QEMU_NEW()

[Avi Kivity]

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()

[Avi Kivity]

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()

[Blue Swirl]

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.

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[Blue Swirl]

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.

Re: [PATCH] Introduce QEMU_NEW()

[Avi Kivity]

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()

[Avi Kivity]

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()

[Blue Swirl]

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.

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[Blue Swirl]

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.

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[Anthony Liguori]

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

>

Re: [PATCH] Introduce QEMU_NEW()

[Max Filippov]

>>>>>> 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()

[Max Filippov]

>>>>>> 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()

[Anthony Liguori]

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: [PATCH] Introduce QEMU_NEW()

[malc]

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:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[malc]

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:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[Avi Kivity]

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()

[Avi Kivity]

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()

[Paolo Bonzini]

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()

[Paolo Bonzini]

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()

[Blue Swirl]

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.

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[Blue Swirl]

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.

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[malc]

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:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[malc]

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:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[Avi Kivity]

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()

[Avi Kivity]

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: [PATCH] Introduce QEMU_NEW()

[Jes Sorensen]

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()

[Jes Sorensen]

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()

[Anthony Liguori]

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()

[Jes Sorensen]

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: [PATCH] Introduce QEMU_NEW()

[Avi Kivity]

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()

[Avi Kivity]

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()

[Jes Sorensen]

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()

[Jes Sorensen]

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()

[Paolo Bonzini]

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()

[Paolo Bonzini]

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: [PATCH] Introduce QEMU_NEW()

[Avi Kivity]

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()

[Avi Kivity]

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()

[Jes Sorensen]

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()

[Jes Sorensen]

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: [PATCH] Introduce QEMU_NEW()

[Avi Kivity]

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

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[Avi Kivity]

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

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[Richard W.M. Jones]

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.

-- 
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/

Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()

[Richard W.M. Jones]

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.

-- 
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/