Re: [PATCH 3/3] Use g_new() & friends where that makes obvious sense

[Markus Armbruster <armbru@redhat.com>]

Alex Bennée <alex.bennee@linaro.org> writes:

> Markus Armbruster <armbru@redhat.com> 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 <armbru@redhat.com>

[...]

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

> <snip>
>> 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->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.
>
> <snip>
>
> 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.bennee@linaro.org>

Thanks!