Re: [PATCH v4 2/7] epoll_ctl: Extract epoll operations

[Laurent Vivier <lvivier@redhat.com>]

On 20/10/2025 03:20, David Gibson wrote:
> On Fri, Oct 17, 2025 at 12:31:24PM +0200, Laurent Vivier wrote:
>> Centralize epoll_add() and epoll_del() helper functions into new
>> epoll_ctl.c/h files.
>>
>> This also moves the union epoll_ref definition from passt.h to
>> epoll_ctl.h where it's more logically placed.
>>
>> The new epoll_add() helper simplifies adding file descriptors to epoll
>> by taking an epoll_ref and events, handling error reporting
>> consistently across all call sites.
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> 
> Concept looks good, some minor details in execution.
> 
>> ---
>>   Makefile     | 22 +++++++++++-----------
>>   epoll_ctl.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>   epoll_ctl.h  | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   icmp.c       |  4 +---
>>   passt.c      |  2 +-
>>   passt.h      | 34 ----------------------------------
>>   pasta.c      |  7 +++----
>>   repair.c     | 14 +++++---------
>>   tap.c        | 13 ++++---------
>>   tcp.c        |  2 +-
>>   tcp_splice.c |  2 +-
>>   udp.c        |  2 +-
>>   udp_flow.c   |  1 +
>>   util.c       | 22 +++-------------------
>>   util.h       |  4 +++-
>>   vhost_user.c |  8 ++------
>>   vu_common.c  |  2 +-
>>   17 files changed, 134 insertions(+), 101 deletions(-)
>>   create mode 100644 epoll_ctl.c
>>   create mode 100644 epoll_ctl.h
...
>> @@ -1327,14 +1327,12 @@ static void tap_backend_show_hints(struct ctx *c)
>>   static void tap_sock_unix_init(const struct ctx *c)
>>   {
>>   	union epoll_ref ref = { .type = EPOLL_TYPE_TAP_LISTEN };
>> -	struct epoll_event ev = { 0 };
>>   
>>   	listen(c->fd_tap_listen, 0);
>>   
>>   	ref.fd = c->fd_tap_listen;
>> -	ev.events = EPOLLIN | EPOLLET;
>> -	ev.data.u64 = ref.u64;
>> -	epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap_listen, &ev);
>> +
>> +	epoll_add(c->epollfd, EPOLLIN | EPOLLET, &ref);
> 
> Preexisting, but we should probably check for errors here.
> 

To do what? die() ? err() to report the place where the error happens?

Thanks,
Laurent