Hi Mark,
On 7/1/25 8:01 PM, Mark Michelson wrote:
>>> +struct ovn_unsynced_datapath *
>>> +ovn_unsynced_datapath_alloc(const char *name, enum ovn_datapath_type
>>> type,
>>> + uint32_t requested_tunnel_key,
>>> + const struct ovsdb_idl_row *nb_row)
>>> +{
>>> + struct ovn_unsynced_datapath *dp = xmalloc(sizeof *dp);
>>> + dp->name = xstrdup(name);
>>> + dp->type = type;
>>> + dp->requested_tunnel_key = requested_tunnel_key;
>>> + dp->nb_row = nb_row;
>>> + smap_init(&dp->external_ids);
>>> +
>>
>> Nit (obviously a personal preference so it's fine to ignore it): I
>> started preferring designated initializers as they ensure that we don't
>> forget zero-ing out not explicitly initialized members. E.g.:
>>
>> *dp = (struct ovn_unsynced_datapath) {
>> .name = xstrdup(name),
>> .type = type,
>> .requested_tunnel_key = requested_tunnel_key,
>> .nb_row = nb_row,
>> .external_ids = SMAP_INITIALIZER(&dp->external_ids),
>> };
>>
>
> The problem with this advice is that it runs contrary to findings that
> were made in early versions of this series.
>
> I initially had used xzalloc() instead of xmalloc(), for the reason you
> had suggested here. But it was suggested that I can use xmalloc() since
> all fields are set in this function, and zeroing the fields beforehand
> is unnecessary.
>
> If I switch to using xmalloc() + struct initializer, it seems like a
> roundabout way of going back to using xzalloc().
>
I don't fully agree with this to be honest. There is, in my opinion, a
difference between:
a. xzalloc() followed by individually setting all fields of the structure
and
b. xmalloc() followed by whole struct initialization with designated
initializers
and
c. xmalloc() followed by individual member initializations
With "a", the memory allocated for the structure is (at least in theory)
written twice.
With both "b" and "c" we write to the memory of the structure once.
However, with "b" there's a guarantee that if we forget to initialize
some members they'll be zeroed so we won't have random garbage there.
From a "performance" perspective I'm quite sure all current compilers
(when run with a decent level of optimizations enabled, e.g., -O2) will
optimize all three options above to the same object code.
> So with that in mind, I don't plan to make the initializer changes you
> suggest here and elsewhere in the review.
Regardless of what I said above, I'm OK with keeping it as it is in your
patch.
Thanks,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
