Hi all!

Here is a refactoring of initialization code, to improve its
readability and get rid of duplication.

v2:
01,03: improve commit msg
14: fix return value for new net_tap_init_one()
15: add return statements to other cases, to not break them
20: new

Below are the initialization flow diagrams showing the changes.

BEFORE REFACTORING:
==================

```
net_init_tap()
    |
    +-- if (tap->fd)
    |   +-- duplicated logic*
    |   +-- net_init_tap_one()
    |
    +-- else if (tap->fds)
    |   +-- for each fd:
    |       +-- duplicated logic*
    |       +-- net_init_tap_one()
    |
    +-- else if (tap->helper)
    |   +-- duplicated logic*
    |   +-- net_init_bridge()
    |
    +-- else (normal case)
        +-- for each queue:
            +-- net_tap_init()
            +-- net_init_tap_one()

net_init_bridge()
    |
    +-- duplicated logic*
    +-- net_tap_fd_init()

net_init_tap_one()
    |
    +-- net_tap_fd_init()

net_tap_init()
    |
    +-- tap_open()

net_tap_fd_init()
    |
    +-- qemu_new_net_client()
    +-- Initialize TAPState

* duplicated logic: set fd nonblocking + probe vnet_hdr
```

AFTER REFACTORING:
=================

```
net_init_tap()
    |
    +-- if (tap->fd)
    |   +-- net_tap_from_monitor_fd()
    |
    +-- else if (tap->fds)
    |   +-- for each fd:
    |       +-- net_tap_from_monitor_fd()
    |
    +-- else if (tap->helper)
    |   +-- net_init_bridge()
    |
    +-- else (normal case)
        +-- net_tap_open()

net_tap_open()
    |
    +-- for each queue:
        +-- net_tap_open_one()

net_tap_open_one()
    |
    +-- tap_open()
    +-- net_tap_fd_init_common()

net_tap_from_monitor_fd()
    |
    +-- net_tap_fd_init_external()

net_tap_fd_init_external()
    |
    +-- net_tap_fd_init_common()

net_init_bridge()
    |
    +-- net_tap_fd_init_external()

net_tap_fd_init_common()
    |
    +-- qemu_new_net_client()
    +-- Initialize TAPState
```

Solved problems:

- duplicated logic to handle external
  file descriptors (set nonblocking, probe vnet_hdr)

- duplication between tap/helper case in
  net_init_tap() and net_init_bridge()

- confusing naming and functionality spread between functions (we had
  net_init_tap(), together with net_tap_init(); also main central
  function was net_init_tap_one(), and part of its logic (not clear
  why) moved to separate net_tap_fd_init()),

- net_init_tap() was just too big

Vladimir Sementsov-Ogievskiy (20):
  net/tap: net_init_tap_one(): add return value
  net/tap: add set_fd_nonblocking() helper
  net/tap: tap_set_sndbuf(): add return value
  net/tap: net_init_tap_one(): drop extra error propagation
  net/tap: net_init_tap_one(): move parameter checking earlier
  net/tap: net_init_tap(): refactor parameter checking
  net/tap: net_init_tap(): drop extra variable vhostfdname
  net/tap: move local variables related to the latter case to else
    branch
  net/tap: use glib strings vector and g_strsplit for fds case
  net/tap: drop extra tap_fd_get_ifname() call
  net/tap: net_init_tap_one(): refactor to use netdev as first arg
  net/tap: net_init_tap_one(): support bridge
  net/tap: net_init_bridge(): support tap
  net/tap: refactor net_tap_init() into net_tap_open_one()
  net/tap: introduce net_tap_open()
  net/tap: introduce net_tap_fd_init_external()
  net/tap: introduce net_tap_from_monitor_fd() helper
  net/tap: split net_tap_setup_vhost() separate function
  net/tap: drop net_tap_fd_init()
  net/tap: introduce net_init_tap_fds()

 net/tap-linux.c |   5 +-
 net/tap.c       | 578 +++++++++++++++++++++++-------------------------
 net/tap_int.h   |   2 +-
 3 files changed, 277 insertions(+), 308 deletions(-)

-- 
2.48.1

Ping!)

Understand, that it's quite big. I can split into 2-3 series, if this helps.

On 23.08.25 19:03, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> Here is a refactoring of initialization code, to improve its
> readability and get rid of duplication.
> 
> v2:
> 01,03: improve commit msg
> 14: fix return value for new net_tap_init_one()
> 15: add return statements to other cases, to not break them
> 20: new
> 
> Below are the initialization flow diagrams showing the changes.
> 
> BEFORE REFACTORING:
> ==================
> 
> ```
> net_init_tap()
>      |
>      +-- if (tap->fd)
>      |   +-- duplicated logic*
>      |   +-- net_init_tap_one()
>      |
>      +-- else if (tap->fds)
>      |   +-- for each fd:
>      |       +-- duplicated logic*
>      |       +-- net_init_tap_one()
>      |
>      +-- else if (tap->helper)
>      |   +-- duplicated logic*
>      |   +-- net_init_bridge()
>      |
>      +-- else (normal case)
>          +-- for each queue:
>              +-- net_tap_init()
>              +-- net_init_tap_one()
> 
> net_init_bridge()
>      |
>      +-- duplicated logic*
>      +-- net_tap_fd_init()
> 
> net_init_tap_one()
>      |
>      +-- net_tap_fd_init()
> 
> net_tap_init()
>      |
>      +-- tap_open()
> 
> net_tap_fd_init()
>      |
>      +-- qemu_new_net_client()
>      +-- Initialize TAPState
> 
> * duplicated logic: set fd nonblocking + probe vnet_hdr
> ```
> 
> AFTER REFACTORING:
> =================
> 
> ```
> net_init_tap()
>      |
>      +-- if (tap->fd)
>      |   +-- net_tap_from_monitor_fd()
>      |
>      +-- else if (tap->fds)
>      |   +-- for each fd:
>      |       +-- net_tap_from_monitor_fd()
>      |
>      +-- else if (tap->helper)
>      |   +-- net_init_bridge()
>      |
>      +-- else (normal case)
>          +-- net_tap_open()
> 
> net_tap_open()
>      |
>      +-- for each queue:
>          +-- net_tap_open_one()
> 
> net_tap_open_one()
>      |
>      +-- tap_open()
>      +-- net_tap_fd_init_common()
> 
> net_tap_from_monitor_fd()
>      |
>      +-- net_tap_fd_init_external()
> 
> net_tap_fd_init_external()
>      |
>      +-- net_tap_fd_init_common()
> 
> net_init_bridge()
>      |
>      +-- net_tap_fd_init_external()
> 
> net_tap_fd_init_common()
>      |
>      +-- qemu_new_net_client()
>      +-- Initialize TAPState
> ```
> 
> Solved problems:
> 
> - duplicated logic to handle external
>    file descriptors (set nonblocking, probe vnet_hdr)
> 
> - duplication between tap/helper case in
>    net_init_tap() and net_init_bridge()
> 
> - confusing naming and functionality spread between functions (we had
>    net_init_tap(), together with net_tap_init(); also main central
>    function was net_init_tap_one(), and part of its logic (not clear
>    why) moved to separate net_tap_fd_init()),
> 
> - net_init_tap() was just too big
> 
> Vladimir Sementsov-Ogievskiy (20):
>    net/tap: net_init_tap_one(): add return value
>    net/tap: add set_fd_nonblocking() helper
>    net/tap: tap_set_sndbuf(): add return value
>    net/tap: net_init_tap_one(): drop extra error propagation
>    net/tap: net_init_tap_one(): move parameter checking earlier
>    net/tap: net_init_tap(): refactor parameter checking
>    net/tap: net_init_tap(): drop extra variable vhostfdname
>    net/tap: move local variables related to the latter case to else
>      branch
>    net/tap: use glib strings vector and g_strsplit for fds case
>    net/tap: drop extra tap_fd_get_ifname() call
>    net/tap: net_init_tap_one(): refactor to use netdev as first arg
>    net/tap: net_init_tap_one(): support bridge
>    net/tap: net_init_bridge(): support tap
>    net/tap: refactor net_tap_init() into net_tap_open_one()
>    net/tap: introduce net_tap_open()
>    net/tap: introduce net_tap_fd_init_external()
>    net/tap: introduce net_tap_from_monitor_fd() helper
>    net/tap: split net_tap_setup_vhost() separate function
>    net/tap: drop net_tap_fd_init()
>    net/tap: introduce net_init_tap_fds()
> 
>   net/tap-linux.c |   5 +-
>   net/tap.c       | 578 +++++++++++++++++++++++-------------------------
>   net/tap_int.h   |   2 +-
>   3 files changed, 277 insertions(+), 308 deletions(-)
> 


-- 
Best regards,
Vladimir

On Mon, Sep 1, 2025 at 10:28 PM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> Ping!)
>
> Understand, that it's quite big. I can split into 2-3 series, if this helps.

I will have a look this week. The size should be fine so no need to split.

Thanks

>
> On 23.08.25 19:03, Vladimir Sementsov-Ogievskiy wrote:
> > Hi all!
> >
> > Here is a refactoring of initialization code, to improve its
> > readability and get rid of duplication.
> >
> > v2:
> > 01,03: improve commit msg
> > 14: fix return value for new net_tap_init_one()
> > 15: add return statements to other cases, to not break them
> > 20: new
> >
> > Below are the initialization flow diagrams showing the changes.
> >
> > BEFORE REFACTORING:
> > ==================
> >
> > ```
> > net_init_tap()
> >      |
> >      +-- if (tap->fd)
> >      |   +-- duplicated logic*
> >      |   +-- net_init_tap_one()
> >      |
> >      +-- else if (tap->fds)
> >      |   +-- for each fd:
> >      |       +-- duplicated logic*
> >      |       +-- net_init_tap_one()
> >      |
> >      +-- else if (tap->helper)
> >      |   +-- duplicated logic*
> >      |   +-- net_init_bridge()
> >      |
> >      +-- else (normal case)
> >          +-- for each queue:
> >              +-- net_tap_init()
> >              +-- net_init_tap_one()
> >
> > net_init_bridge()
> >      |
> >      +-- duplicated logic*
> >      +-- net_tap_fd_init()
> >
> > net_init_tap_one()
> >      |
> >      +-- net_tap_fd_init()
> >
> > net_tap_init()
> >      |
> >      +-- tap_open()
> >
> > net_tap_fd_init()
> >      |
> >      +-- qemu_new_net_client()
> >      +-- Initialize TAPState
> >
> > * duplicated logic: set fd nonblocking + probe vnet_hdr
> > ```
> >
> > AFTER REFACTORING:
> > =================
> >
> > ```
> > net_init_tap()
> >      |
> >      +-- if (tap->fd)
> >      |   +-- net_tap_from_monitor_fd()
> >      |
> >      +-- else if (tap->fds)
> >      |   +-- for each fd:
> >      |       +-- net_tap_from_monitor_fd()
> >      |
> >      +-- else if (tap->helper)
> >      |   +-- net_init_bridge()
> >      |
> >      +-- else (normal case)
> >          +-- net_tap_open()
> >
> > net_tap_open()
> >      |
> >      +-- for each queue:
> >          +-- net_tap_open_one()
> >
> > net_tap_open_one()
> >      |
> >      +-- tap_open()
> >      +-- net_tap_fd_init_common()
> >
> > net_tap_from_monitor_fd()
> >      |
> >      +-- net_tap_fd_init_external()
> >
> > net_tap_fd_init_external()
> >      |
> >      +-- net_tap_fd_init_common()
> >
> > net_init_bridge()
> >      |
> >      +-- net_tap_fd_init_external()
> >
> > net_tap_fd_init_common()
> >      |
> >      +-- qemu_new_net_client()
> >      +-- Initialize TAPState
> > ```
> >
> > Solved problems:
> >
> > - duplicated logic to handle external
> >    file descriptors (set nonblocking, probe vnet_hdr)
> >
> > - duplication between tap/helper case in
> >    net_init_tap() and net_init_bridge()
> >
> > - confusing naming and functionality spread between functions (we had
> >    net_init_tap(), together with net_tap_init(); also main central
> >    function was net_init_tap_one(), and part of its logic (not clear
> >    why) moved to separate net_tap_fd_init()),
> >
> > - net_init_tap() was just too big
> >
> > Vladimir Sementsov-Ogievskiy (20):
> >    net/tap: net_init_tap_one(): add return value
> >    net/tap: add set_fd_nonblocking() helper
> >    net/tap: tap_set_sndbuf(): add return value
> >    net/tap: net_init_tap_one(): drop extra error propagation
> >    net/tap: net_init_tap_one(): move parameter checking earlier
> >    net/tap: net_init_tap(): refactor parameter checking
> >    net/tap: net_init_tap(): drop extra variable vhostfdname
> >    net/tap: move local variables related to the latter case to else
> >      branch
> >    net/tap: use glib strings vector and g_strsplit for fds case
> >    net/tap: drop extra tap_fd_get_ifname() call
> >    net/tap: net_init_tap_one(): refactor to use netdev as first arg
> >    net/tap: net_init_tap_one(): support bridge
> >    net/tap: net_init_bridge(): support tap
> >    net/tap: refactor net_tap_init() into net_tap_open_one()
> >    net/tap: introduce net_tap_open()
> >    net/tap: introduce net_tap_fd_init_external()
> >    net/tap: introduce net_tap_from_monitor_fd() helper
> >    net/tap: split net_tap_setup_vhost() separate function
> >    net/tap: drop net_tap_fd_init()
> >    net/tap: introduce net_init_tap_fds()
> >
> >   net/tap-linux.c |   5 +-
> >   net/tap.c       | 578 +++++++++++++++++++++++-------------------------
> >   net/tap_int.h   |   2 +-
> >   3 files changed, 277 insertions(+), 308 deletions(-)
> >
>
>
> --
> Best regards,
> Vladimir
>

Tested this series of patches with virtio-net regression
tests,everything works fine.

Tested-by: Lei Yang <leiyang@redhat.com>

On Sun, Aug 24, 2025 at 12:03 AM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> Hi all!
>
> Here is a refactoring of initialization code, to improve its
> readability and get rid of duplication.
>
> v2:
> 01,03: improve commit msg
> 14: fix return value for new net_tap_init_one()
> 15: add return statements to other cases, to not break them
> 20: new
>
> Below are the initialization flow diagrams showing the changes.
>
> BEFORE REFACTORING:
> ==================
>
> ```
> net_init_tap()
>     |
>     +-- if (tap->fd)
>     |   +-- duplicated logic*
>     |   +-- net_init_tap_one()
>     |
>     +-- else if (tap->fds)
>     |   +-- for each fd:
>     |       +-- duplicated logic*
>     |       +-- net_init_tap_one()
>     |
>     +-- else if (tap->helper)
>     |   +-- duplicated logic*
>     |   +-- net_init_bridge()
>     |
>     +-- else (normal case)
>         +-- for each queue:
>             +-- net_tap_init()
>             +-- net_init_tap_one()
>
> net_init_bridge()
>     |
>     +-- duplicated logic*
>     +-- net_tap_fd_init()
>
> net_init_tap_one()
>     |
>     +-- net_tap_fd_init()
>
> net_tap_init()
>     |
>     +-- tap_open()
>
> net_tap_fd_init()
>     |
>     +-- qemu_new_net_client()
>     +-- Initialize TAPState
>
> * duplicated logic: set fd nonblocking + probe vnet_hdr
> ```
>
> AFTER REFACTORING:
> =================
>
> ```
> net_init_tap()
>     |
>     +-- if (tap->fd)
>     |   +-- net_tap_from_monitor_fd()
>     |
>     +-- else if (tap->fds)
>     |   +-- for each fd:
>     |       +-- net_tap_from_monitor_fd()
>     |
>     +-- else if (tap->helper)
>     |   +-- net_init_bridge()
>     |
>     +-- else (normal case)
>         +-- net_tap_open()
>
> net_tap_open()
>     |
>     +-- for each queue:
>         +-- net_tap_open_one()
>
> net_tap_open_one()
>     |
>     +-- tap_open()
>     +-- net_tap_fd_init_common()
>
> net_tap_from_monitor_fd()
>     |
>     +-- net_tap_fd_init_external()
>
> net_tap_fd_init_external()
>     |
>     +-- net_tap_fd_init_common()
>
> net_init_bridge()
>     |
>     +-- net_tap_fd_init_external()
>
> net_tap_fd_init_common()
>     |
>     +-- qemu_new_net_client()
>     +-- Initialize TAPState
> ```
>
> Solved problems:
>
> - duplicated logic to handle external
>   file descriptors (set nonblocking, probe vnet_hdr)
>
> - duplication between tap/helper case in
>   net_init_tap() and net_init_bridge()
>
> - confusing naming and functionality spread between functions (we had
>   net_init_tap(), together with net_tap_init(); also main central
>   function was net_init_tap_one(), and part of its logic (not clear
>   why) moved to separate net_tap_fd_init()),
>
> - net_init_tap() was just too big
>
> Vladimir Sementsov-Ogievskiy (20):
>   net/tap: net_init_tap_one(): add return value
>   net/tap: add set_fd_nonblocking() helper
>   net/tap: tap_set_sndbuf(): add return value
>   net/tap: net_init_tap_one(): drop extra error propagation
>   net/tap: net_init_tap_one(): move parameter checking earlier
>   net/tap: net_init_tap(): refactor parameter checking
>   net/tap: net_init_tap(): drop extra variable vhostfdname
>   net/tap: move local variables related to the latter case to else
>     branch
>   net/tap: use glib strings vector and g_strsplit for fds case
>   net/tap: drop extra tap_fd_get_ifname() call
>   net/tap: net_init_tap_one(): refactor to use netdev as first arg
>   net/tap: net_init_tap_one(): support bridge
>   net/tap: net_init_bridge(): support tap
>   net/tap: refactor net_tap_init() into net_tap_open_one()
>   net/tap: introduce net_tap_open()
>   net/tap: introduce net_tap_fd_init_external()
>   net/tap: introduce net_tap_from_monitor_fd() helper
>   net/tap: split net_tap_setup_vhost() separate function
>   net/tap: drop net_tap_fd_init()
>   net/tap: introduce net_init_tap_fds()
>
>  net/tap-linux.c |   5 +-
>  net/tap.c       | 578 +++++++++++++++++++++++-------------------------
>  net/tap_int.h   |   2 +-
>  3 files changed, 277 insertions(+), 308 deletions(-)
>
> --
> 2.48.1
>

On 25.08.25 06:26, Lei Yang wrote:
> Tested this series of patches with virtio-net regression
> tests,everything works fine.
> 
> Tested-by: Lei Yang<leiyang@redhat.com>

Thanks!

-- 
Best regards,
Vladimir