On 7/2/25 3:55 AM, Dumitru Ceara wrote:
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.
This is why I considered options a and b to be equivalent. I have
typically used xzalloc() as my method of choice for ensuring unspecified
fields are zeroed. I tend to use it even when all fields will be
assigned, since it future-proofs against additions to the struct.
It seems like option b works as a way to hedge in case the compiler
can't optimize the assignment. I think I'll migrate to using your
suggestion of xmalloc() + initializer from now on, and I will go ahead
and use it in these patches as well.
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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
