On 2025/07/15 23:52, Paolo Abeni wrote:
On 7/15/25 8:36 AM, Akihiko Odaki wrote:
On 2025/07/11 22:02, Paolo Abeni wrote:
The set_offload() argument list is already pretty long and
we are going to introduce soon a bunch of additional offloads.
Replace the offload arguments with a single struct and update
all the relevant call-sites.
No functional changes intended.
Signed-off-by: Paolo Abeni <pab...@redhat.com>
---
Note: I maintained the struct usage as opposed to uint64_t bitmask usage
as suggested by Akihiko, because the latter feel a bit more invasive.
I think a bitmask will be invasive to the same extent with the current
version; most part of this change comes from the parameter passing,
which does not depend on the representation of the parameter.
Do you have strong feeling WRT the bitmask usage?
Another argument vs the bitmask usage is that it will requires some
extra input validation of the selected offload bits (most of them don't
make sense in this context).
I don't think such a validation is necessary.
There is practically no chance to have a wrong bit set by mistake when
callers specify bits with macros prefixed with VIRTIO_NET_O_GUEST_.
There will be a compilation error if the caller specify a offload bit
that doesn't exist.
It is also obvious if a caller specified something unrelated (i.e., not
prefixed with VIRTIO_NET_O_GUEST_). A real downside here would be that
we will need to type VIRTIO_NET_O_GUEST_ each time referring an offload
bit; such a redundancy does not exist with struct because the type
system knows the bits bound to the type.
That said, 1) I prefer bitmasks much over struct though 2) I will be in
favor of merging this series if everything else gets sorted out while
the struct remains.
I'll explain the reason for 1) first:
There are both a downside and an upside with bitmasks. The downside is
the redundancy of syntax, which I have just pointed out. The upside is
the consistency with virtio's offload configuration, which defines the
functionality of set_offload().
I consider the following two factors in such a trade-off scenario:
a) The balance between the downside and upside
b) Prior examples in the code base
Regarding a), I think the upside outweighs the downside but its extent
is small.
b) matters more for this particular case; there are bunch of examples
that use bitmasks in "include", excluding "include/hw" but there are
none of structs that are dedicated for bools. Consistency matters for a
big code base like QEMU so I want a good reason when making an exception.
The reasoning behind 2) is that having this patch is still better than
the status quo.
Regards,
Akihiko Odaki
