Re: [PATCH 3/3] Use g_new() & friends where that makes obvious sense
Markus Armbruster writes:
> Philippe Mathieu-Daudé writes:
>
>> On 15/3/22 14:59, Markus Armbruster wrote:
>>> Alex Bennée writes:
>>>
Markus Armbruster writes:
> g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer,
> for two reasons. One, it catches multiplication overflowing size_t.
> Two, it returns T * rather than void *, which lets the compiler catch
> more type errors.
>
> diff --git a/semihosting/config.c b/semihosting/config.c
> index 137171b717..6d48ec9566 100644
> --- a/semihosting/config.c
> +++ b/semihosting/config.c
> @@ -98,7 +98,7 @@ static int add_semihosting_arg(void *opaque,
> if (strcmp(name, "arg") == 0) {
> s->argc++;
> /* one extra element as g_strjoinv() expects NULL-terminated
> array */
> -s->argv = g_realloc(s->argv, (s->argc + 1) * sizeof(void *));
> +s->argv = g_renew(void *, s->argv, s->argc + 1);
This did indeed break CI because s->argv is an array of *char:
../semihosting/config.c:101:17: error: assignment to ‘const char **’ from
incompatible pointer type ‘void **’ [-Werror=incompatible-pointer-types]
101 | s->argv = g_renew(void *, s->argv, s->argc + 1);
| ^
cc1: all warnings being treated as errors
So it did the job of type checking but failed to build ;-)
>>>
>>> You found a hole in my compile testing, thanks!
>>>
>>> I got confused about the configuration of my build trees. Catching such
>>> mistakes is what CI is for :)
>>
>> FYI Alex fixed this here:
>> https://lore.kernel.org/qemu-devel/20220315121251.2280317-8-alex.ben...@linaro.org/
>>
>> So your series could go on top (modulo the Coverity change).
>
> I dropped this hunk in v2.
>
> Whether my v2 or Alex's series goes in first doesn't matter.
That's great. Thanks for finding the ugliness in the first place ;-)
--
Alex Bennée
Re: [PATCH 3/3] Use g_new() & friends where that makes obvious sense
Philippe Mathieu-Daudé writes:
> On 15/3/22 14:59, Markus Armbruster wrote:
>> Alex Bennée writes:
>>
>>> Markus Armbruster writes:
>>>
g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer,
for two reasons. One, it catches multiplication overflowing size_t.
Two, it returns T * rather than void *, which lets the compiler catch
more type errors.
>>>
diff --git a/semihosting/config.c b/semihosting/config.c
index 137171b717..6d48ec9566 100644
--- a/semihosting/config.c
+++ b/semihosting/config.c
@@ -98,7 +98,7 @@ static int add_semihosting_arg(void *opaque,
if (strcmp(name, "arg") == 0) {
s->argc++;
/* one extra element as g_strjoinv() expects NULL-terminated
array */
-s->argv = g_realloc(s->argv, (s->argc + 1) * sizeof(void *));
+s->argv = g_renew(void *, s->argv, s->argc + 1);
>>>
>>> This did indeed break CI because s->argv is an array of *char:
>>>
>>> ../semihosting/config.c:101:17: error: assignment to ‘const char **’ from
>>> incompatible pointer type ‘void **’ [-Werror=incompatible-pointer-types]
>>>101 | s->argv = g_renew(void *, s->argv, s->argc + 1);
>>>| ^
>>> cc1: all warnings being treated as errors
>>>
>>> So it did the job of type checking but failed to build ;-)
>>
>> You found a hole in my compile testing, thanks!
>>
>> I got confused about the configuration of my build trees. Catching such
>> mistakes is what CI is for :)
>
> FYI Alex fixed this here:
> https://lore.kernel.org/qemu-devel/20220315121251.2280317-8-alex.ben...@linaro.org/
>
> So your series could go on top (modulo the Coverity change).
I dropped this hunk in v2.
Whether my v2 or Alex's series goes in first doesn't matter.
Re: [PATCH 3/3] Use g_new() & friends where that makes obvious sense
On 15/3/22 14:59, Markus Armbruster wrote:
Alex Bennée writes:
Markus Armbruster writes:
g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer,
for two reasons. One, it catches multiplication overflowing size_t.
Two, it returns T * rather than void *, which lets the compiler catch
more type errors.
diff --git a/semihosting/config.c b/semihosting/config.c
index 137171b717..6d48ec9566 100644
--- a/semihosting/config.c
+++ b/semihosting/config.c
@@ -98,7 +98,7 @@ static int add_semihosting_arg(void *opaque,
if (strcmp(name, "arg") == 0) {
s->argc++;
/* one extra element as g_strjoinv() expects NULL-terminated array */
-s->argv = g_realloc(s->argv, (s->argc + 1) * sizeof(void *));
+s->argv = g_renew(void *, s->argv, s->argc + 1);
This did indeed break CI because s->argv is an array of *char:
../semihosting/config.c:101:17: error: assignment to ‘const char **’ from
incompatible pointer type ‘void **’ [-Werror=incompatible-pointer-types]
101 | s->argv = g_renew(void *, s->argv, s->argc + 1);
| ^
cc1: all warnings being treated as errors
So it did the job of type checking but failed to build ;-)
You found a hole in my compile testing, thanks!
I got confused about the configuration of my build trees. Catching such
mistakes is what CI is for :)
FYI Alex fixed this here:
https://lore.kernel.org/qemu-devel/20220315121251.2280317-8-alex.ben...@linaro.org/
So your series could go on top (modulo the Coverity change).
Re: [PATCH 3/3] Use g_new() & friends where that makes obvious sense
Eric Blake writes:
> On Mon, Mar 14, 2022 at 05:01:08PM +0100, Markus Armbruster wrote:
>> g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer,
>> for two reasons. One, it catches multiplication overflowing size_t.
>> Two, it returns T * rather than void *, which lets the compiler catch
>> more type errors.
>>
>> This commit only touches allocations with size arguments of the form
>> sizeof(T).
>>
>> Patch created mechanically with:
>>
>> $ spatch --in-place --sp-file scripts/coccinelle/use-g_new-etc.cocci \
>> --macro-file scripts/cocci-macro-file.h FILES...
>>
>> Signed-off-by: Markus Armbruster
>> ---
>
> I agree that this is mechanical, but...
>
>
>> qga/commands-win32.c | 8 ++---
>> qga/commands.c | 2 +-
>> qom/qom-qmp-cmds.c | 2 +-
>> replay/replay-char.c | 4 +--
>> replay/replay-events.c | 10 +++---
>> scripts/coverity-scan/model.c| 2 +-
>
> ...are we sure we want to touch this particular file?
Good catch!
>> diff --git a/scripts/coverity-scan/model.c b/scripts/coverity-scan/model.c
>> index 9d4fba53d9..30bea672e1 100644
>> --- a/scripts/coverity-scan/model.c
>> +++ b/scripts/coverity-scan/model.c
>> @@ -356,7 +356,7 @@ int g_poll (GPollFD *fds, unsigned nfds, int timeout)
>> typedef struct _GIOChannel GIOChannel;
>> GIOChannel *g_io_channel_unix_new(int fd)
>> {
>> -GIOChannel *c = g_malloc0(sizeof(GIOChannel));
>> +GIOChannel *c = g_new0(GIOChannel, 1);
>> __coverity_escape__(fd);
>> return c;
>> }
>
> Our model has a definition of g_malloc0(), but I'm not sure whether
> Coverity picks up the macro g_new0() in the same manner.
I believe it does, by parsing the macro definition from the header.
Regardless, I'd prefer to keep model.c self-contained. I'll drop this
hunk.
Thanks!
Re: [PATCH 3/3] Use g_new() & friends where that makes obvious sense
Alex Bennée writes:
> Markus Armbruster writes:
>
>> g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer,
>> for two reasons. One, it catches multiplication overflowing size_t.
>> Two, it returns T * rather than void *, which lets the compiler catch
>> more type errors.
>>
>
>> diff --git a/semihosting/config.c b/semihosting/config.c
>> index 137171b717..6d48ec9566 100644
>> --- a/semihosting/config.c
>> +++ b/semihosting/config.c
>> @@ -98,7 +98,7 @@ static int add_semihosting_arg(void *opaque,
>> if (strcmp(name, "arg") == 0) {
>> s->argc++;
>> /* one extra element as g_strjoinv() expects NULL-terminated array
>> */
>> -s->argv = g_realloc(s->argv, (s->argc + 1) * sizeof(void *));
>> +s->argv = g_renew(void *, s->argv, s->argc + 1);
>
> This did indeed break CI because s->argv is an array of *char:
>
> ../semihosting/config.c:101:17: error: assignment to ‘const char **’ from
> incompatible pointer type ‘void **’ [-Werror=incompatible-pointer-types]
> 101 | s->argv = g_renew(void *, s->argv, s->argc + 1);
> | ^
> cc1: all warnings being treated as errors
>
> So it did the job of type checking but failed to build ;-)
You found a hole in my compile testing, thanks!
I got confused about the configuration of my build trees. Catching such
mistakes is what CI is for :)
Re: [PATCH 3/3] Use g_new() & friends where that makes obvious sense
Alex Bennée writes:
> Markus Armbruster writes:
>
>> g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer,
>> for two reasons. One, it catches multiplication overflowing size_t.
>> Two, it returns T * rather than void *, which lets the compiler catch
>> more type errors.
>>
>> This commit only touches allocations with size arguments of the form
>> sizeof(T).
>>
>> Patch created mechanically with:
>>
>> $ spatch --in-place --sp-file scripts/coccinelle/use-g_new-etc.cocci \
>> --macro-file scripts/cocci-macro-file.h FILES...
>>
>> Signed-off-by: Markus Armbruster
[...]
>> --- a/hw/pci/pcie_sriov.c
>> +++ b/hw/pci/pcie_sriov.c
>> @@ -177,7 +177,7 @@ static void register_vfs(PCIDevice *dev)
>> assert(sriov_cap > 0);
>> num_vfs = pci_get_word(dev->config + sriov_cap + PCI_SRIOV_NUM_VF);
>>
>> -dev->exp.sriov_pf.vf = g_malloc(sizeof(PCIDevice *) * num_vfs);
>> +dev->exp.sriov_pf.vf = g_new(PCIDevice *, num_vfs);
>> assert(dev->exp.sriov_pf.vf);
>
> So what I find confusing about the conversion of sizeof(foo *) is that
> while the internal sizeof in g_new is unaffected I think the casting
> ends up as
>
> foo **
Yes. g_malloc(...) returns void *. g_new(T, ...) returns T *.
> but I guess the compiler would have complained so maybe I don't
> understand the magic enough.
It doesn't complain here because dev->exp.sriov_pf.vf is of type
PCIDevice **:
struct PCIESriovPF {
uint16_t num_vfs; /* Number of virtual functions created */
uint8_t vf_bar_type[PCI_NUM_REGIONS]; /* Store type for each VF bar */
const char *vfname; /* Reference to the device type used for the VFs */
PCIDevice **vf; /* Pointer to an array of num_vfs VF devices */
};
It does complain when the types don't match, as shown in PATCH 2.
>
>> index 42130667a7..598e6adc5e 100644
>> --- a/hw/rdma/vmw/pvrdma_dev_ring.c
>> +++ b/hw/rdma/vmw/pvrdma_dev_ring.c
>> @@ -41,7 +41,7 @@ int pvrdma_ring_init(PvrdmaRing *ring, const char *name,
>> PCIDevice *dev,
>> qatomic_set(>ring_state->cons_head, 0);
>> */
>> ring->npages = npages;
>> -ring->pages = g_malloc0(npages * sizeof(void *));
>> +ring->pages = g_new0(void *, npages);
>
> At least here ring->pages agrees about void ** being the result.
>
>
>
> So other than my queries about the sizeof(foo *) which I'd like someone
> to assuage me of my concerns it looks like the script has done a
> thorough mechanical job as all good scripts should ;-)
>
> Reviewed-by: Alex Bennée
Thanks!
Re: [PATCH 3/3] Use g_new() & friends where that makes obvious sense
* Markus Armbruster (arm...@redhat.com) wrote:
> g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer,
> for two reasons. One, it catches multiplication overflowing size_t.
> Two, it returns T * rather than void *, which lets the compiler catch
> more type errors.
>
> This commit only touches allocations with size arguments of the form
> sizeof(T).
>
> Patch created mechanically with:
>
> $ spatch --in-place --sp-file scripts/coccinelle/use-g_new-etc.cocci \
>--macro-file scripts/cocci-macro-file.h FILES...
>
> Signed-off-by: Markus Armbruster
Just a small patch then...
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index d65e744af9..aace12a787 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -91,7 +91,7 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
> {
> int i;
> int64_t dirty_rate = DirtyStat.dirty_rate;
> -struct DirtyRateInfo *info = g_malloc0(sizeof(DirtyRateInfo));
> +struct DirtyRateInfo *info = g_new0(DirtyRateInfo, 1);
> DirtyRateVcpuList *head = NULL, **tail =
>
> info->status = CalculatingState;
> @@ -112,7 +112,7 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
> info->sample_pages = 0;
> info->has_vcpu_dirty_rate = true;
> for (i = 0; i < DirtyStat.dirty_ring.nvcpu; i++) {
> -DirtyRateVcpu *rate = g_malloc0(sizeof(DirtyRateVcpu));
> +DirtyRateVcpu *rate = g_new0(DirtyRateVcpu, 1);
> rate->id = DirtyStat.dirty_ring.rates[i].id;
> rate->dirty_rate = DirtyStat.dirty_ring.rates[i].dirty_rate;
> QAPI_LIST_APPEND(tail, rate);
> diff --git a/migration/multifd-zlib.c b/migration/multifd-zlib.c
> index aba1c88a0c..3a7ae44485 100644
> --- a/migration/multifd-zlib.c
> +++ b/migration/multifd-zlib.c
> @@ -43,7 +43,7 @@ struct zlib_data {
> */
> static int zlib_send_setup(MultiFDSendParams *p, Error **errp)
> {
> -struct zlib_data *z = g_malloc0(sizeof(struct zlib_data));
> +struct zlib_data *z = g_new0(struct zlib_data, 1);
> z_stream *zs = >zs;
>
> zs->zalloc = Z_NULL;
> @@ -164,7 +164,7 @@ static int zlib_send_prepare(MultiFDSendParams *p, Error
> **errp)
> */
> static int zlib_recv_setup(MultiFDRecvParams *p, Error **errp)
> {
> -struct zlib_data *z = g_malloc0(sizeof(struct zlib_data));
> +struct zlib_data *z = g_new0(struct zlib_data, 1);
> z_stream *zs = >zs;
>
> p->data = z;
> diff --git a/migration/ram.c b/migration/ram.c
> index 170e522a1f..3532f64ecb 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2059,7 +2059,7 @@ int ram_save_queue_pages(const char *rbname, ram_addr_t
> start, ram_addr_t len)
> }
>
> struct RAMSrcPageRequest *new_entry =
> -g_malloc0(sizeof(struct RAMSrcPageRequest));
> +g_new0(struct RAMSrcPageRequest, 1);
> new_entry->rb = ramblock;
> new_entry->offset = start;
> new_entry->len = len;
For migration:
Acked-by: Dr. David Alan Gilbert
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH 3/3] Use g_new() & friends where that makes obvious sense
On Mon, Mar 14, 2022 at 05:01:08PM +0100, Markus Armbruster wrote:
> g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer,
> for two reasons. One, it catches multiplication overflowing size_t.
> Two, it returns T * rather than void *, which lets the compiler catch
> more type errors.
>
> This commit only touches allocations with size arguments of the form
> sizeof(T).
>
> Patch created mechanically with:
>
> $ spatch --in-place --sp-file scripts/coccinelle/use-g_new-etc.cocci \
>--macro-file scripts/cocci-macro-file.h FILES...
>
> Signed-off-by: Markus Armbruster
> ---
I agree that this is mechanical, but...
> qga/commands-win32.c | 8 ++---
> qga/commands.c | 2 +-
> qom/qom-qmp-cmds.c | 2 +-
> replay/replay-char.c | 4 +--
> replay/replay-events.c | 10 +++---
> scripts/coverity-scan/model.c| 2 +-
...are we sure we want to touch this particular file?
> diff --git a/scripts/coverity-scan/model.c b/scripts/coverity-scan/model.c
> index 9d4fba53d9..30bea672e1 100644
> --- a/scripts/coverity-scan/model.c
> +++ b/scripts/coverity-scan/model.c
> @@ -356,7 +356,7 @@ int g_poll (GPollFD *fds, unsigned nfds, int timeout)
> typedef struct _GIOChannel GIOChannel;
> GIOChannel *g_io_channel_unix_new(int fd)
> {
> -GIOChannel *c = g_malloc0(sizeof(GIOChannel));
> +GIOChannel *c = g_new0(GIOChannel, 1);
> __coverity_escape__(fd);
> return c;
> }
Our model has a definition of g_malloc0(), but I'm not sure whether
Coverity picks up the macro g_new0() in the same manner.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH 3/3] Use g_new() & friends where that makes obvious sense
Markus Armbruster writes:
> g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer,
> for two reasons. One, it catches multiplication overflowing size_t.
> Two, it returns T * rather than void *, which lets the compiler catch
> more type errors.
>
> diff --git a/semihosting/config.c b/semihosting/config.c
> index 137171b717..6d48ec9566 100644
> --- a/semihosting/config.c
> +++ b/semihosting/config.c
> @@ -98,7 +98,7 @@ static int add_semihosting_arg(void *opaque,
> if (strcmp(name, "arg") == 0) {
> s->argc++;
> /* one extra element as g_strjoinv() expects NULL-terminated array */
> -s->argv = g_realloc(s->argv, (s->argc + 1) * sizeof(void *));
> +s->argv = g_renew(void *, s->argv, s->argc + 1);
This did indeed break CI because s->argv is an array of *char:
../semihosting/config.c:101:17: error: assignment to ‘const char **’ from
incompatible pointer type ‘void **’ [-Werror=incompatible-pointer-types]
101 | s->argv = g_renew(void *, s->argv, s->argc + 1);
| ^
cc1: all warnings being treated as errors
So it did the job of type checking but failed to build ;-)
--
Alex Bennée
Re: [PATCH 3/3] Use g_new() & friends where that makes obvious sense
Markus Armbruster writes:
> g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer,
> for two reasons. One, it catches multiplication overflowing size_t.
> Two, it returns T * rather than void *, which lets the compiler catch
> more type errors.
>
> This commit only touches allocations with size arguments of the form
> sizeof(T).
>
> Patch created mechanically with:
>
> $ spatch --in-place --sp-file scripts/coccinelle/use-g_new-etc.cocci \
>--macro-file scripts/cocci-macro-file.h FILES...
>
> Signed-off-by: Markus Armbruster
> --- a/audio/jackaudio.c
> +++ b/audio/jackaudio.c
> @@ -97,9 +97,9 @@ static void qjack_buffer_create(QJackBuffer *buffer, int
> channels, int frames)
> buffer->used = 0;
> buffer->rptr = 0;
> buffer->wptr = 0;
> -buffer->data = g_malloc(channels * sizeof(float *));
> +buffer->data = g_new(float *, channels);
> for (int i = 0; i < channels; ++i) {
> -buffer->data[i] = g_malloc(frames * sizeof(float));
> +buffer->data[i] = g_new(float, frames);
Are these actually buffers of pointers to floats? I guess I leave that
to the JACK experts...
> }
> }
>
> @@ -453,7 +453,7 @@ static int qjack_client_init(QJackClient *c)
> jack_on_shutdown(c->client, qjack_shutdown, c);
>
> /* allocate and register the ports */
> -c->port = g_malloc(sizeof(jack_port_t *) * c->nchannels);
> +c->port = g_new(jack_port_t *, c->nchannels);
> for (int i = 0; i < c->nchannels; ++i) {
I guess JACK just likes double indirection...
>
> char port_name[16];
> --- a/hw/pci/pcie_sriov.c
> +++ b/hw/pci/pcie_sriov.c
> @@ -177,7 +177,7 @@ static void register_vfs(PCIDevice *dev)
> assert(sriov_cap > 0);
> num_vfs = pci_get_word(dev->config + sriov_cap + PCI_SRIOV_NUM_VF);
>
> -dev->exp.sriov_pf.vf = g_malloc(sizeof(PCIDevice *) * num_vfs);
> +dev->exp.sriov_pf.vf = g_new(PCIDevice *, num_vfs);
> assert(dev->exp.sriov_pf.vf);
So what I find confusing about the conversion of sizeof(foo *) is that
while the internal sizeof in g_new is unaffected I think the casting
ends up as
foo **
but I guess the compiler would have complained so maybe I don't
understand the magic enough.
> index 42130667a7..598e6adc5e 100644
> --- a/hw/rdma/vmw/pvrdma_dev_ring.c
> +++ b/hw/rdma/vmw/pvrdma_dev_ring.c
> @@ -41,7 +41,7 @@ int pvrdma_ring_init(PvrdmaRing *ring, const char *name,
> PCIDevice *dev,
> qatomic_set(>ring_state->cons_head, 0);
> */
> ring->npages = npages;
> -ring->pages = g_malloc0(npages * sizeof(void *));
> +ring->pages = g_new0(void *, npages);
At least here ring->pages agrees about void ** being the result.
So other than my queries about the sizeof(foo *) which I'd like someone
to assuage me of my concerns it looks like the script has done a
thorough mechanical job as all good scripts should ;-)
Reviewed-by: Alex Bennée
--
Alex Bennée
Re: [PATCH 3/3] Use g_new() & friends where that makes obvious sense
On Mon, Mar 14, 2022 at 05:52:32PM +0100, Markus Armbruster wrote: > Peter Maydell writes: > > > On Mon, 14 Mar 2022 at 16:01, Markus Armbruster wrote: > >> > >> g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer, > >> for two reasons. One, it catches multiplication overflowing size_t. > >> Two, it returns T * rather than void *, which lets the compiler catch > >> more type errors. > >> > >> This commit only touches allocations with size arguments of the form > >> sizeof(T). > >> > >> Patch created mechanically with: > >> > >> $ spatch --in-place --sp-file scripts/coccinelle/use-g_new-etc.cocci \ > >> --macro-file scripts/cocci-macro-file.h FILES... > >> > >> Signed-off-by: Markus Armbruster > >> --- > > > >> 104 files changed, 197 insertions(+), 202 deletions(-) > > > > I'm not going to say you must split this patch up. I'm just going to > > say that I personally am not looking at it, because it's too big > > for me to deal with. > > As with all big but trivial Coccinelle patches, reviewing the Coccinelle > script and a reasonably representative sample of its output is almost > certainly a better use of reviewer time than attempting to get all the > patches reviewed. They are mind-numbingly dull! > > For what it's worth, we've used this script several times before. Last > in commit bdd81addf4. This Coccinelle is simple enough to understand, that I'd suggest that once we merge the Coccinelle script itself, then for ongoing usage, its output can be considered effectively pre-reviewed. The reviewer can just re-run the Coccinelle script themselves to prove it has the same output as the submitter claims, to validate no manual changes are hidden in the middle of the automated patch. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 3/3] Use g_new() & friends where that makes obvious sense
On 3/14/22 17:01, Markus Armbruster wrote: g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer, for two reasons. One, it catches multiplication overflowing size_t. Two, it returns T * rather than void *, which lets the compiler catch more type errors. This commit only touches allocations with size arguments of the form sizeof(T). Patch created mechanically with: $ spatch --in-place --sp-file scripts/coccinelle/use-g_new-etc.cocci \ --macro-file scripts/cocci-macro-file.h FILES... Signed-off-by: Markus Armbruster --- include/qemu/timer.h | 2 +- accel/kvm/kvm-all.c | 6 ++-- accel/tcg/tcg-accel-ops-mttcg.c | 2 +- accel/tcg/tcg-accel-ops-rr.c | 4 +-- audio/audio.c| 4 +-- audio/audio_legacy.c | 6 ++-- audio/dsoundaudio.c | 2 +- audio/jackaudio.c| 6 ++-- audio/paaudio.c | 4 +-- backends/cryptodev.c | 2 +- contrib/vhost-user-gpu/vhost-user-gpu.c | 2 +- cpus-common.c| 4 +-- dump/dump.c | 2 +- hw/acpi/hmat.c | 2 +- hw/audio/intel-hda.c | 2 +- hw/char/parallel.c | 2 +- hw/char/riscv_htif.c | 2 +- hw/char/virtio-serial-bus.c | 6 ++-- hw/core/irq.c| 2 +- hw/core/reset.c | 2 +- hw/display/pxa2xx_lcd.c | 2 +- hw/display/tc6393xb.c| 2 +- hw/display/virtio-gpu.c | 4 +-- hw/display/xenfb.c | 4 +-- hw/dma/rc4030.c | 4 +-- hw/i2c/core.c| 4 +-- hw/i2c/i2c_mux_pca954x.c | 2 +- hw/i386/amd_iommu.c | 4 +-- hw/i386/intel_iommu.c| 2 +- hw/i386/xen/xen-hvm.c| 10 +++--- hw/i386/xen/xen-mapcache.c | 14 hw/input/lasips2.c | 2 +- hw/input/pckbd.c | 2 +- hw/input/ps2.c | 4 +-- hw/input/pxa2xx_keypad.c | 2 +- hw/input/tsc2005.c | 3 +- hw/intc/riscv_aclint.c | 6 ++-- hw/intc/xics.c | 2 +- hw/m68k/virt.c | 2 +- hw/mips/mipssim.c| 2 +- hw/misc/applesmc.c | 2 +- hw/misc/imx6_src.c | 2 +- hw/misc/ivshmem.c| 4 +-- hw/net/virtio-net.c | 4 +-- hw/nvme/ns.c | 2 +- hw/pci-host/pnv_phb3.c | 2 +- hw/pci-host/pnv_phb4.c | 2 +- hw/pci/pcie_sriov.c | 2 +- hw/ppc/e500.c| 2 +- hw/ppc/ppc.c | 8 ++--- hw/ppc/ppc405_boards.c | 4 +-- hw/ppc/ppc405_uc.c | 18 +- hw/ppc/ppc4xx_devs.c | 2 +- hw/ppc/ppc_booke.c | 4 +-- hw/ppc/spapr.c | 2 +- hw/ppc/spapr_events.c| 2 +- hw/ppc/spapr_hcall.c | 2 +- hw/ppc/spapr_numa.c | 3 +- hw/rdma/vmw/pvrdma_dev_ring.c| 2 +- hw/rdma/vmw/pvrdma_qp_ops.c | 6 ++-- hw/sh4/r2d.c | 4 +-- hw/sh4/sh7750.c | 2 +- hw/sparc/leon3.c | 2 +- hw/sparc64/sparc64.c | 4 +-- hw/timer/arm_timer.c | 2 +- hw/timer/slavio_timer.c | 2 +- hw/vfio/pci.c| 4 +-- hw/vfio/platform.c | 4 +-- hw/virtio/virtio-crypto.c| 2 +- hw/virtio/virtio-iommu.c | 2 +- hw/virtio/virtio.c | 5 ++- hw/xtensa/xtfpga.c | 2 +- linux-user/syscall.c | 2 +- migration/dirtyrate.c| 4 +-- migration/multifd-zlib.c | 4 +-- migration/ram.c | 2 +- monitor/misc.c | 2 +- monitor/qmp-cmds.c | 2 +- qga/commands-win32.c | 8 ++--- qga/commands.c | 2 +- qom/qom-qmp-cmds.c | 2 +- replay/replay-char.c | 4 +-- replay/replay-events.c | 10 +++--- scripts/coverity-scan/model.c| 2 +- semihosting/config.c | 2 +- softmmu/bootdevice.c
Re: [PATCH 3/3] Use g_new() & friends where that makes obvious sense
Peter Maydell writes: > On Mon, 14 Mar 2022 at 16:01, Markus Armbruster wrote: >> >> g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer, >> for two reasons. One, it catches multiplication overflowing size_t. >> Two, it returns T * rather than void *, which lets the compiler catch >> more type errors. >> >> This commit only touches allocations with size arguments of the form >> sizeof(T). >> >> Patch created mechanically with: >> >> $ spatch --in-place --sp-file scripts/coccinelle/use-g_new-etc.cocci \ >> --macro-file scripts/cocci-macro-file.h FILES... >> >> Signed-off-by: Markus Armbruster >> --- > >> 104 files changed, 197 insertions(+), 202 deletions(-) > > I'm not going to say you must split this patch up. I'm just going to > say that I personally am not looking at it, because it's too big > for me to deal with. As with all big but trivial Coccinelle patches, reviewing the Coccinelle script and a reasonably representative sample of its output is almost certainly a better use of reviewer time than attempting to get all the patches reviewed. They are mind-numbingly dull! For what it's worth, we've used this script several times before. Last in commit bdd81addf4.
Re: [PATCH 3/3] Use g_new() & friends where that makes obvious sense
On Mon, 14 Mar 2022 at 16:01, Markus Armbruster wrote: > > g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer, > for two reasons. One, it catches multiplication overflowing size_t. > Two, it returns T * rather than void *, which lets the compiler catch > more type errors. > > This commit only touches allocations with size arguments of the form > sizeof(T). > > Patch created mechanically with: > > $ spatch --in-place --sp-file scripts/coccinelle/use-g_new-etc.cocci \ > --macro-file scripts/cocci-macro-file.h FILES... > > Signed-off-by: Markus Armbruster > --- > 104 files changed, 197 insertions(+), 202 deletions(-) I'm not going to say you must split this patch up. I'm just going to say that I personally am not looking at it, because it's too big for me to deal with. -- PMM
[PATCH 3/3] Use g_new() & friends where that makes obvious sense
g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer, for two reasons. One, it catches multiplication overflowing size_t. Two, it returns T * rather than void *, which lets the compiler catch more type errors. This commit only touches allocations with size arguments of the form sizeof(T). Patch created mechanically with: $ spatch --in-place --sp-file scripts/coccinelle/use-g_new-etc.cocci \ --macro-file scripts/cocci-macro-file.h FILES... Signed-off-by: Markus Armbruster --- include/qemu/timer.h | 2 +- accel/kvm/kvm-all.c | 6 ++-- accel/tcg/tcg-accel-ops-mttcg.c | 2 +- accel/tcg/tcg-accel-ops-rr.c | 4 +-- audio/audio.c| 4 +-- audio/audio_legacy.c | 6 ++-- audio/dsoundaudio.c | 2 +- audio/jackaudio.c| 6 ++-- audio/paaudio.c | 4 +-- backends/cryptodev.c | 2 +- contrib/vhost-user-gpu/vhost-user-gpu.c | 2 +- cpus-common.c| 4 +-- dump/dump.c | 2 +- hw/acpi/hmat.c | 2 +- hw/audio/intel-hda.c | 2 +- hw/char/parallel.c | 2 +- hw/char/riscv_htif.c | 2 +- hw/char/virtio-serial-bus.c | 6 ++-- hw/core/irq.c| 2 +- hw/core/reset.c | 2 +- hw/display/pxa2xx_lcd.c | 2 +- hw/display/tc6393xb.c| 2 +- hw/display/virtio-gpu.c | 4 +-- hw/display/xenfb.c | 4 +-- hw/dma/rc4030.c | 4 +-- hw/i2c/core.c| 4 +-- hw/i2c/i2c_mux_pca954x.c | 2 +- hw/i386/amd_iommu.c | 4 +-- hw/i386/intel_iommu.c| 2 +- hw/i386/xen/xen-hvm.c| 10 +++--- hw/i386/xen/xen-mapcache.c | 14 hw/input/lasips2.c | 2 +- hw/input/pckbd.c | 2 +- hw/input/ps2.c | 4 +-- hw/input/pxa2xx_keypad.c | 2 +- hw/input/tsc2005.c | 3 +- hw/intc/riscv_aclint.c | 6 ++-- hw/intc/xics.c | 2 +- hw/m68k/virt.c | 2 +- hw/mips/mipssim.c| 2 +- hw/misc/applesmc.c | 2 +- hw/misc/imx6_src.c | 2 +- hw/misc/ivshmem.c| 4 +-- hw/net/virtio-net.c | 4 +-- hw/nvme/ns.c | 2 +- hw/pci-host/pnv_phb3.c | 2 +- hw/pci-host/pnv_phb4.c | 2 +- hw/pci/pcie_sriov.c | 2 +- hw/ppc/e500.c| 2 +- hw/ppc/ppc.c | 8 ++--- hw/ppc/ppc405_boards.c | 4 +-- hw/ppc/ppc405_uc.c | 18 +- hw/ppc/ppc4xx_devs.c | 2 +- hw/ppc/ppc_booke.c | 4 +-- hw/ppc/spapr.c | 2 +- hw/ppc/spapr_events.c| 2 +- hw/ppc/spapr_hcall.c | 2 +- hw/ppc/spapr_numa.c | 3 +- hw/rdma/vmw/pvrdma_dev_ring.c| 2 +- hw/rdma/vmw/pvrdma_qp_ops.c | 6 ++-- hw/sh4/r2d.c | 4 +-- hw/sh4/sh7750.c | 2 +- hw/sparc/leon3.c | 2 +- hw/sparc64/sparc64.c | 4 +-- hw/timer/arm_timer.c | 2 +- hw/timer/slavio_timer.c | 2 +- hw/vfio/pci.c| 4 +-- hw/vfio/platform.c | 4 +-- hw/virtio/virtio-crypto.c| 2 +- hw/virtio/virtio-iommu.c | 2 +- hw/virtio/virtio.c | 5 ++- hw/xtensa/xtfpga.c | 2 +- linux-user/syscall.c | 2 +- migration/dirtyrate.c| 4 +-- migration/multifd-zlib.c | 4 +-- migration/ram.c | 2 +- monitor/misc.c | 2 +- monitor/qmp-cmds.c | 2 +- qga/commands-win32.c | 8 ++--- qga/commands.c | 2 +- qom/qom-qmp-cmds.c | 2 +- replay/replay-char.c | 4 +-- replay/replay-events.c | 10 +++--- scripts/coverity-scan/model.c| 2 +- semihosting/config.c | 2 +- softmmu/bootdevice.c | 4 +-- softmmu/dma-helpers.c| 4 +-- softmmu/memory_mapping.c | 2 +-
