On Tue, Nov 18, 2025 at 01:13:20PM +0100, Markus Armbruster wrote: > Daniel P. Berrangé <[email protected]> writes: > > > On Mon, Nov 17, 2025 at 02:58:37PM +0100, Markus Armbruster wrote: > >> Markus Armbruster <[email protected]> writes: > >> > >> > Daniel P. Berrangé <[email protected]> writes: > >> > > >> >> On Thu, Aug 07, 2025 at 03:14:56PM +0200, Markus Armbruster wrote: > >> >>> Three functions in ebpf_rss.h take an Error ** argument and return > >> >>> bool. > >> >>> Good. > >> >>> > >> >>> They can all fail without setting an error. Not good. > >> >>> > >> >>> The failures without error are: > >> >>> > >> >>> * All three stubs in ebpf_rss-stub.c always. Oversight? > >> >> > >> >> Opps, yes, we really should have added error_setg() calls for diagnosis > >> >> if someone tries to use eBPF when QEMU build has it disabled. > >> > >> Easy enough, but... > >> > >> > Some stubs exist only to mollify the linker. They are not meant to be > >> > called. They should abort(), optionally with lipstick. > >> > > >> > Other stubs are called and should fail nicely. > >> > > >> > Can you tell me offhand which kind these are? > >> > >> If calling these stubs is possible, I'd like to know how I can get them > >> called, so I can test the errors I add. > >> > >> If calling is not possible, I'd rather add abort()s. > >> > >> I tried to figure out whether calling is possible, but it ended in > >> confusion. Can you help? > > > > * ebpf_rss_set_all > > > > Is called from virtio_net_attach_ebpf_rss > > The call is unreachable if ebpf_rss_is_loaded returns false > > Stub for ebpf_rss_is_loaded always returns false > > > > => ebpf_rss_set_all stub is unreachable > > Then the non-stub ebpf_rss_set_all() has a useless check of > ebpf_rss_is_loaded() with an unreachable error message. > > > * ebpf_rss_load_fds, ebpf_rss_load > > > > Is called from virtio_net_load_ebpf_fds, which is called from > > virtio_net_load_ebpf > > > > The call to virtio_net_load_ebpf_fds is unreachable if > > virtio_net_attach_ebpf_to_backend fails > > > > virtio_net_attach_ebpf_to_backend fails if set_steering_ebpf > > fails > > > > set_steering_ebpf fails if ioctl(fd, TUNSETSTEERINGEBPF...) > > fails on Linux; all non-Linux impls of ebpf_rss_load_fds > > return -1 > > > > It is theoretically p9ossible to build QEMU without EBPF > > while both glibc & the kernel support TUNSETSTEERINGEBPF ioctl > > > > => ebpf_rss_load_fds, ebpf_rss_load are reachable in stubs > > So: > > * ebpf_rss_load() and ebpf_rss_load_fds() need a suitable error_setg(). > > * For ebpf_rss_set_all(), we have two sane options: > > - Declare ebpf_rss_is_loaded() a precondition, drop the useless check > from the non-stub version, abort() in the stub. > > - Keep the useless check and error in the non-stub version, add an > equally useless error to the stub. > > Got a preference?
Not my code, but I'd go for declaring ebpf_rss_is_loaded a precond. > >> >>> * Non-stub ebpf_rss_load() when ebpf_rss_is_loaded(). Are these > >> >>> reachable? > >> >> > >> >> This scenario should never happen, and we should add a call like > >> >> > >> >> error_setg(errp, "eBPF program is already loaded"); > >> >> > >> >> to report it correctly. > >> > > >> > Is it a programming error when it happens? > >> > >> This question is still open as well. > > > > I'd consider it a programming error. I don't think we have a code > > path that could trigger it currently. > > Then the proper fix is replacing the flawed check by an assertion. With 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 :|
