[PATCH v3 00/19] TAP initialization refactoring

Vladimir Sementsov-Ogievskiy posted 19 patches 2 days, 23 hours ago
Failed in applying to current master (apply log)
net/tap-linux.c |   5 +-
net/tap.c       | 553 ++++++++++++++++++++++--------------------------
net/tap_int.h   |   2 +-
3 files changed, 263 insertions(+), 297 deletions(-)
[PATCH v3 00/19] TAP initialization refactoring
Posted by Vladimir Sementsov-Ogievskiy 2 days, 23 hours ago
Hi all!

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

v3:
- rebase on top of [PATCH 00/10] io: deal with blocking/non-blocking fds
- improve some commit messages
- add t-b marks by Lei Yang (hope, they still counts after rebasing :)

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 (19):
  net/tap: net_init_tap_one(): add return value
  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       | 553 ++++++++++++++++++++++--------------------------
 net/tap_int.h   |   2 +-
 3 files changed, 263 insertions(+), 297 deletions(-)

-- 
2.48.1
Re: [PATCH v3 00/19] TAP initialization refactoring
Posted by Lei Yang 1 day, 21 hours ago
Tested the current series of patches, mixed with patches from series
[1] and [2], and the virtio-net regression tests passed. I also tested
local VM migration under multiple NIC queues enabled and disabled, it
also passed.

[1] https://patchwork.ozlabs.org/project/qemu-devel/cover/20250903094411.1029449-1-vsementsov@yandex-team.ru/
[2] https://patchwork.ozlabs.org/project/qemu-devel/cover/20250903133706.1177633-1-vsementsov@yandex-team.ru/

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

On Wed, Sep 3, 2025 at 8:49 PM 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.
>
> v3:
> - rebase on top of [PATCH 00/10] io: deal with blocking/non-blocking fds
> - improve some commit messages
> - add t-b marks by Lei Yang (hope, they still counts after rebasing :)
>
> 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 (19):
>   net/tap: net_init_tap_one(): add return value
>   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       | 553 ++++++++++++++++++++++--------------------------
>  net/tap_int.h   |   2 +-
>  3 files changed, 263 insertions(+), 297 deletions(-)
>
> --
> 2.48.1
>