* [PATCH v2 00/20] TAP initialization refactoring
@ 2025-08-23 16:03 Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 01/20] net/tap: net_init_tap_one(): add return value Vladimir Sementsov-Ogievskiy
` (21 more replies)
0 siblings, 22 replies; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-08-23 16:03 UTC (permalink / raw)
To: jasowang; +Cc: qemu-devel, vsementsov, leiyang, steven.sistare
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 01/20] net/tap: net_init_tap_one(): add return value
2025-08-23 16:03 [PATCH v2 00/20] TAP initialization refactoring Vladimir Sementsov-Ogievskiy
@ 2025-08-23 16:03 ` Vladimir Sementsov-Ogievskiy
2025-09-03 4:04 ` Jason Wang
2025-08-23 16:03 ` [PATCH v2 02/20] net/tap: add set_fd_nonblocking() helper Vladimir Sementsov-Ogievskiy
` (20 subsequent siblings)
21 siblings, 1 reply; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-08-23 16:03 UTC (permalink / raw)
To: jasowang; +Cc: qemu-devel, vsementsov, leiyang, steven.sistare
To avoid error propagation, let's follow common recommendation to
use return value together with errp.
Probably, it would also be good to use bool as a return type
(switching to true/false as success/failure instead of 0/-1). But
seems almost all functions (including a lot of them with errp
argument) have 0/-1 semantics in net/, so making exclusions doesn't
seem good. If we want such a switch, we should update the whole
net/ directory.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
net/tap.c | 55 +++++++++++++++++++++++++------------------------------
1 file changed, 25 insertions(+), 30 deletions(-)
diff --git a/net/tap.c b/net/tap.c
index f7df702f97..531ef75e91 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -680,11 +680,11 @@ static int net_tap_init(const NetdevTapOptions *tap, int *vnet_hdr,
#define MAX_TAP_QUEUES 1024
-static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
- const char *model, const char *name,
- const char *ifname, const char *script,
- const char *downscript, const char *vhostfdname,
- int vnet_hdr, int fd, Error **errp)
+static int net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
+ const char *model, const char *name,
+ const char *ifname, const char *script,
+ const char *downscript, const char *vhostfdname,
+ int vnet_hdr, int fd, Error **errp)
{
Error *err = NULL;
TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
@@ -765,10 +765,11 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
goto failed;
}
- return;
+ return 0;
failed:
qemu_del_net_client(&s->nc);
+ return -1;
}
static int get_fds(char *str, char *fds[], int max)
@@ -805,7 +806,6 @@ int net_init_tap(const Netdev *netdev, const char *name,
/* for the no-fd, no-helper case */
const char *script;
const char *downscript;
- Error *err = NULL;
const char *vhostfdname;
char ifname[128];
int ret = 0;
@@ -852,12 +852,10 @@ int net_init_tap(const Netdev *netdev, const char *name,
return -1;
}
- net_init_tap_one(tap, peer, "tap", name, NULL,
- script, downscript,
- vhostfdname, vnet_hdr, fd, &err);
- if (err) {
- error_propagate(errp, err);
- close(fd);
+ ret = net_init_tap_one(tap, peer, "tap", name, NULL,
+ script, downscript,
+ vhostfdname, vnet_hdr, fd, errp);
+ if (ret < 0) {
return -1;
}
} else if (tap->fds) {
@@ -915,12 +913,11 @@ int net_init_tap(const Netdev *netdev, const char *name,
goto free_fail;
}
- net_init_tap_one(tap, peer, "tap", name, ifname,
- script, downscript,
- tap->vhostfds ? vhost_fds[i] : NULL,
- vnet_hdr, fd, &err);
- if (err) {
- error_propagate(errp, err);
+ ret = net_init_tap_one(tap, peer, "tap", name, ifname,
+ script, downscript,
+ tap->vhostfds ? vhost_fds[i] : NULL,
+ vnet_hdr, fd, errp);
+ if (ret < 0) {
ret = -1;
goto free_fail;
}
@@ -961,11 +958,10 @@ free_fail:
return -1;
}
- net_init_tap_one(tap, peer, "bridge", name, ifname,
- script, downscript, vhostfdname,
- vnet_hdr, fd, &err);
- if (err) {
- error_propagate(errp, err);
+ ret = net_init_tap_one(tap, peer, "bridge", name, ifname,
+ script, downscript, vhostfdname,
+ vnet_hdr, fd, errp);
+ if (ret < 0) {
close(fd);
return -1;
}
@@ -1006,12 +1002,11 @@ free_fail:
}
}
- net_init_tap_one(tap, peer, "tap", name, ifname,
- i >= 1 ? "no" : script,
- i >= 1 ? "no" : downscript,
- vhostfdname, vnet_hdr, fd, &err);
- if (err) {
- error_propagate(errp, err);
+ ret = net_init_tap_one(tap, peer, "tap", name, ifname,
+ i >= 1 ? "no" : script,
+ i >= 1 ? "no" : downscript,
+ vhostfdname, vnet_hdr, fd, errp);
+ if (ret < 0) {
close(fd);
return -1;
}
--
2.48.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2 02/20] net/tap: add set_fd_nonblocking() helper
2025-08-23 16:03 [PATCH v2 00/20] TAP initialization refactoring Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 01/20] net/tap: net_init_tap_one(): add return value Vladimir Sementsov-Ogievskiy
@ 2025-08-23 16:03 ` Vladimir Sementsov-Ogievskiy
2025-09-02 12:32 ` Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 03/20] net/tap: tap_set_sndbuf(): add return value Vladimir Sementsov-Ogievskiy
` (19 subsequent siblings)
21 siblings, 1 reply; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-08-23 16:03 UTC (permalink / raw)
To: jasowang; +Cc: qemu-devel, vsementsov, leiyang, steven.sistare
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
net/tap.c | 34 +++++++++++++++++++---------------
1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/net/tap.c b/net/tap.c
index 531ef75e91..ba2731ee8a 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -609,6 +609,18 @@ static int net_bridge_run_helper(const char *helper, const char *bridge,
}
}
+static bool set_fd_nonblocking(int fd, const char *note, Error **errp)
+{
+ bool ok = g_unix_set_fd_nonblocking(fd, true, NULL);
+
+ if (!ok) {
+ error_setg_errno(errp, errno, "Failed to set fd %d (%s) nonblocking",
+ fd, note);
+ }
+
+ return ok;
+}
+
int net_init_bridge(const Netdev *netdev, const char *name,
NetClientState *peer, Error **errp)
{
@@ -627,10 +639,10 @@ int net_init_bridge(const Netdev *netdev, const char *name,
return -1;
}
- if (!g_unix_set_fd_nonblocking(fd, true, NULL)) {
- error_setg_errno(errp, errno, "Failed to set FD nonblocking");
+ if (!set_fd_nonblocking(fd, name, errp)) {
return -1;
}
+
vnet_hdr = tap_probe_vnet_hdr(fd, errp);
if (vnet_hdr < 0) {
close(fd);
@@ -729,9 +741,7 @@ static int net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
error_propagate(errp, err);
goto failed;
}
- if (!g_unix_set_fd_nonblocking(vhostfd, true, NULL)) {
- error_setg_errno(errp, errno, "%s: Can't use file descriptor %d",
- name, fd);
+ if (!set_fd_nonblocking(vhostfd, vhostfdname, errp)) {
goto failed;
}
} else {
@@ -741,8 +751,7 @@ static int net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
"tap: open vhost char device failed");
goto failed;
}
- if (!g_unix_set_fd_nonblocking(vhostfd, true, NULL)) {
- error_setg_errno(errp, errno, "Failed to set FD nonblocking");
+ if (!set_fd_nonblocking(vhostfd, "opened /dev/vhost-net", errp)) {
goto failed;
}
}
@@ -839,9 +848,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
return -1;
}
- if (!g_unix_set_fd_nonblocking(fd, true, NULL)) {
- error_setg_errno(errp, errno, "%s: Can't use file descriptor %d",
- name, fd);
+ if (!set_fd_nonblocking(fd, tap->fd, errp)) {
close(fd);
return -1;
}
@@ -893,10 +900,8 @@ int net_init_tap(const Netdev *netdev, const char *name,
goto free_fail;
}
- if (!g_unix_set_fd_nonblocking(fd, true, NULL)) {
+ if (!set_fd_nonblocking(fd, fds[i], errp)) {
ret = -1;
- error_setg_errno(errp, errno, "%s: Can't use file descriptor %d",
- name, fd);
goto free_fail;
}
@@ -948,8 +953,7 @@ free_fail:
return -1;
}
- if (!g_unix_set_fd_nonblocking(fd, true, NULL)) {
- error_setg_errno(errp, errno, "Failed to set FD nonblocking");
+ if (!set_fd_nonblocking(fd, name, errp)) {
return -1;
}
vnet_hdr = tap_probe_vnet_hdr(fd, errp);
--
2.48.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2 03/20] net/tap: tap_set_sndbuf(): add return value
2025-08-23 16:03 [PATCH v2 00/20] TAP initialization refactoring Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 01/20] net/tap: net_init_tap_one(): add return value Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 02/20] net/tap: add set_fd_nonblocking() helper Vladimir Sementsov-Ogievskiy
@ 2025-08-23 16:03 ` Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 04/20] net/tap: net_init_tap_one(): drop extra error propagation Vladimir Sementsov-Ogievskiy
` (18 subsequent siblings)
21 siblings, 0 replies; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-08-23 16:03 UTC (permalink / raw)
To: jasowang; +Cc: qemu-devel, vsementsov, leiyang, steven.sistare
Follow common recommendations to avoid error propagation.
Probably, it would also be good to use bool as a return type
(switching to true/false as success/failure instead of 0/-1). But
seems almost all functions (including a lot of them with errp
argument) have 0/-1 semantics in net/, so making exclusions doesn't
seem good. If we want such a switch, we should update the whole
net/ directory.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
net/tap-linux.c | 5 ++++-
net/tap.c | 4 +---
net/tap_int.h | 2 +-
3 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/net/tap-linux.c b/net/tap-linux.c
index 22ec2f45d2..3625379c58 100644
--- a/net/tap-linux.c
+++ b/net/tap-linux.c
@@ -140,7 +140,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
*/
#define TAP_DEFAULT_SNDBUF 0
-void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
+int tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
{
int sndbuf;
@@ -154,7 +154,10 @@ void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
if (ioctl(fd, TUNSETSNDBUF, &sndbuf) == -1 && tap->has_sndbuf) {
error_setg_errno(errp, errno, "TUNSETSNDBUF ioctl failed");
+ return -1;
}
+
+ return 0;
}
int tap_probe_vnet_hdr(int fd, Error **errp)
diff --git a/net/tap.c b/net/tap.c
index ba2731ee8a..0258ac8574 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -702,9 +702,7 @@ static int net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
int vhostfd;
- tap_set_sndbuf(s->fd, tap, &err);
- if (err) {
- error_propagate(errp, err);
+ if (tap_set_sndbuf(s->fd, tap, errp) < 0) {
goto failed;
}
diff --git a/net/tap_int.h b/net/tap_int.h
index 8857ff299d..d21acf8486 100644
--- a/net/tap_int.h
+++ b/net/tap_int.h
@@ -33,7 +33,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen);
-void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp);
+int tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp);
int tap_probe_vnet_hdr(int fd, Error **errp);
int tap_probe_has_ufo(int fd);
int tap_probe_has_uso(int fd);
--
2.48.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2 04/20] net/tap: net_init_tap_one(): drop extra error propagation
2025-08-23 16:03 [PATCH v2 00/20] TAP initialization refactoring Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2025-08-23 16:03 ` [PATCH v2 03/20] net/tap: tap_set_sndbuf(): add return value Vladimir Sementsov-Ogievskiy
@ 2025-08-23 16:03 ` Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 05/20] net/tap: net_init_tap_one(): move parameter checking earlier Vladimir Sementsov-Ogievskiy
` (17 subsequent siblings)
21 siblings, 0 replies; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-08-23 16:03 UTC (permalink / raw)
To: jasowang; +Cc: qemu-devel, vsementsov, leiyang, steven.sistare
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
net/tap.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/net/tap.c b/net/tap.c
index 0258ac8574..58c3318b1c 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -698,7 +698,6 @@ static int net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
const char *downscript, const char *vhostfdname,
int vnet_hdr, int fd, Error **errp)
{
- Error *err = NULL;
TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
int vhostfd;
@@ -734,9 +733,8 @@ static int net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
}
if (vhostfdname) {
- vhostfd = monitor_fd_param(monitor_cur(), vhostfdname, &err);
+ vhostfd = monitor_fd_param(monitor_cur(), vhostfdname, errp);
if (vhostfd == -1) {
- error_propagate(errp, err);
goto failed;
}
if (!set_fd_nonblocking(vhostfd, vhostfdname, errp)) {
--
2.48.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2 05/20] net/tap: net_init_tap_one(): move parameter checking earlier
2025-08-23 16:03 [PATCH v2 00/20] TAP initialization refactoring Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2025-08-23 16:03 ` [PATCH v2 04/20] net/tap: net_init_tap_one(): drop extra error propagation Vladimir Sementsov-Ogievskiy
@ 2025-08-23 16:03 ` Vladimir Sementsov-Ogievskiy
2025-09-03 4:18 ` Jason Wang
2025-08-23 16:03 ` [PATCH v2 06/20] net/tap: net_init_tap(): refactor parameter checking Vladimir Sementsov-Ogievskiy
` (16 subsequent siblings)
21 siblings, 1 reply; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-08-23 16:03 UTC (permalink / raw)
To: jasowang; +Cc: qemu-devel, vsementsov, leiyang, steven.sistare
Let's keep all similar argument checking in net_init_tap() function.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
net/tap.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/net/tap.c b/net/tap.c
index 58c3318b1c..3fe99ef63f 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -765,9 +765,6 @@ static int net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
"vhost-net requested but could not be initialized");
goto failed;
}
- } else if (vhostfdname) {
- error_setg(errp, "vhostfd(s)= is not valid without vhost");
- goto failed;
}
return 0;
@@ -829,6 +826,11 @@ int net_init_tap(const Netdev *netdev, const char *name,
return -1;
}
+ if (tap->has_vhost && !tap->vhost && (tap->vhostfds || tap->vhostfd)) {
+ error_setg(errp, "vhostfd(s)= is not valid without vhost");
+ return -1;
+ }
+
if (tap->fd) {
if (tap->ifname || tap->script || tap->downscript ||
tap->has_vnet_hdr || tap->helper || tap->has_queues ||
--
2.48.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2 06/20] net/tap: net_init_tap(): refactor parameter checking
2025-08-23 16:03 [PATCH v2 00/20] TAP initialization refactoring Vladimir Sementsov-Ogievskiy
` (4 preceding siblings ...)
2025-08-23 16:03 ` [PATCH v2 05/20] net/tap: net_init_tap_one(): move parameter checking earlier Vladimir Sementsov-Ogievskiy
@ 2025-08-23 16:03 ` Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 07/20] net/tap: net_init_tap(): drop extra variable vhostfdname Vladimir Sementsov-Ogievskiy
` (15 subsequent siblings)
21 siblings, 0 replies; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-08-23 16:03 UTC (permalink / raw)
To: jasowang; +Cc: qemu-devel, vsementsov, leiyang, steven.sistare
Unite duplicated checks of different code paths.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
net/tap.c | 29 ++++++++++++++---------------
1 file changed, 14 insertions(+), 15 deletions(-)
diff --git a/net/tap.c b/net/tap.c
index 3fe99ef63f..042f9fd01a 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -831,12 +831,17 @@ int net_init_tap(const Netdev *netdev, const char *name,
return -1;
}
+ if ((tap->fd || tap->fds || tap->helper) &&
+ (tap->ifname || tap->script || tap->downscript ||
+ tap->has_vnet_hdr || tap->has_queues)) {
+ error_setg(errp, "ifname=, script=, downscript=, vnet_hdr=, "
+ "queues=, are invalid with fd=/fds=/helper=");
+ return -1;
+ }
+
if (tap->fd) {
- if (tap->ifname || tap->script || tap->downscript ||
- tap->has_vnet_hdr || tap->helper || tap->has_queues ||
- tap->fds || tap->vhostfds) {
- error_setg(errp, "ifname=, script=, downscript=, vnet_hdr=, "
- "helper=, queues=, fds=, and vhostfds= "
+ if (tap->helper || tap->fds || tap->vhostfds) {
+ error_setg(errp, "helper=, fds=, and vhostfds= "
"are invalid with fd=");
return -1;
}
@@ -868,12 +873,8 @@ int net_init_tap(const Netdev *netdev, const char *name,
char **vhost_fds;
int nfds = 0, nvhosts = 0;
- if (tap->ifname || tap->script || tap->downscript ||
- tap->has_vnet_hdr || tap->helper || tap->has_queues ||
- tap->vhostfd) {
- error_setg(errp, "ifname=, script=, downscript=, vnet_hdr=, "
- "helper=, queues=, and vhostfd= "
- "are invalid with fds=");
+ if (tap->helper || tap->vhostfd) {
+ error_setg(errp, "helper= and vhostfd= are invalid with fds=");
return -1;
}
@@ -937,10 +938,8 @@ free_fail:
g_free(vhost_fds);
return ret;
} else if (tap->helper) {
- if (tap->ifname || tap->script || tap->downscript ||
- tap->has_vnet_hdr || tap->has_queues || tap->vhostfds) {
- error_setg(errp, "ifname=, script=, downscript=, vnet_hdr=, "
- "queues=, and vhostfds= are invalid with helper=");
+ if (tap->vhostfds) {
+ error_setg(errp, "vhostfds= is invalid with helper=");
return -1;
}
--
2.48.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2 07/20] net/tap: net_init_tap(): drop extra variable vhostfdname
2025-08-23 16:03 [PATCH v2 00/20] TAP initialization refactoring Vladimir Sementsov-Ogievskiy
` (5 preceding siblings ...)
2025-08-23 16:03 ` [PATCH v2 06/20] net/tap: net_init_tap(): refactor parameter checking Vladimir Sementsov-Ogievskiy
@ 2025-08-23 16:03 ` Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 08/20] net/tap: move local variables related to the latter case to else branch Vladimir Sementsov-Ogievskiy
` (14 subsequent siblings)
21 siblings, 0 replies; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-08-23 16:03 UTC (permalink / raw)
To: jasowang; +Cc: qemu-devel, vsementsov, leiyang, steven.sistare
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
net/tap.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/net/tap.c b/net/tap.c
index 042f9fd01a..ade58826c8 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -808,14 +808,12 @@ int net_init_tap(const Netdev *netdev, const char *name,
/* for the no-fd, no-helper case */
const char *script;
const char *downscript;
- const char *vhostfdname;
char ifname[128];
int ret = 0;
assert(netdev->type == NET_CLIENT_DRIVER_TAP);
tap = &netdev->u.tap;
queues = tap->has_queues ? tap->queues : 1;
- vhostfdname = tap->vhostfd;
script = tap->script;
downscript = tap->downscript;
@@ -864,7 +862,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
ret = net_init_tap_one(tap, peer, "tap", name, NULL,
script, downscript,
- vhostfdname, vnet_hdr, fd, errp);
+ tap->vhostfd, vnet_hdr, fd, errp);
if (ret < 0) {
return -1;
}
@@ -960,7 +958,7 @@ free_fail:
}
ret = net_init_tap_one(tap, peer, "bridge", name, ifname,
- script, downscript, vhostfdname,
+ script, downscript, tap->vhostfd,
vnet_hdr, fd, errp);
if (ret < 0) {
close(fd);
@@ -1006,7 +1004,7 @@ free_fail:
ret = net_init_tap_one(tap, peer, "tap", name, ifname,
i >= 1 ? "no" : script,
i >= 1 ? "no" : downscript,
- vhostfdname, vnet_hdr, fd, errp);
+ tap->vhostfd, vnet_hdr, fd, errp);
if (ret < 0) {
close(fd);
return -1;
--
2.48.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2 08/20] net/tap: move local variables related to the latter case to else branch
2025-08-23 16:03 [PATCH v2 00/20] TAP initialization refactoring Vladimir Sementsov-Ogievskiy
` (6 preceding siblings ...)
2025-08-23 16:03 ` [PATCH v2 07/20] net/tap: net_init_tap(): drop extra variable vhostfdname Vladimir Sementsov-Ogievskiy
@ 2025-08-23 16:03 ` Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 09/20] net/tap: use glib strings vector and g_strsplit for fds case Vladimir Sementsov-Ogievskiy
` (13 subsequent siblings)
21 siblings, 0 replies; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-08-23 16:03 UTC (permalink / raw)
To: jasowang; +Cc: qemu-devel, vsementsov, leiyang, steven.sistare
This makes more obvious, where the variables make sense.
Note that ifname, script and downscript variables are not used in
net_init_tap_one() for fd/fds/helper cases.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
net/tap.c | 27 ++++++++++++---------------
1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/net/tap.c b/net/tap.c
index ade58826c8..80ec54f914 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -803,19 +803,11 @@ static int get_fds(char *str, char *fds[], int max)
int net_init_tap(const Netdev *netdev, const char *name,
NetClientState *peer, Error **errp)
{
- const NetdevTapOptions *tap;
- int fd, vnet_hdr = 0, i = 0, queues;
- /* for the no-fd, no-helper case */
- const char *script;
- const char *downscript;
- char ifname[128];
+ const NetdevTapOptions *tap = &netdev->u.tap;
+ int fd, vnet_hdr = 0, i = 0;
int ret = 0;
assert(netdev->type == NET_CLIENT_DRIVER_TAP);
- tap = &netdev->u.tap;
- queues = tap->has_queues ? tap->queues : 1;
- script = tap->script;
- downscript = tap->downscript;
/* QEMU hubs do not support multiqueue tap, in this case peer is set.
* For -netdev, peer is always NULL. */
@@ -861,7 +853,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
}
ret = net_init_tap_one(tap, peer, "tap", name, NULL,
- script, downscript,
+ NULL, NULL,
tap->vhostfd, vnet_hdr, fd, errp);
if (ret < 0) {
return -1;
@@ -915,8 +907,8 @@ int net_init_tap(const Netdev *netdev, const char *name,
goto free_fail;
}
- ret = net_init_tap_one(tap, peer, "tap", name, ifname,
- script, downscript,
+ ret = net_init_tap_one(tap, peer, "tap", name, NULL,
+ NULL, NULL,
tap->vhostfds ? vhost_fds[i] : NULL,
vnet_hdr, fd, errp);
if (ret < 0) {
@@ -957,16 +949,21 @@ free_fail:
return -1;
}
- ret = net_init_tap_one(tap, peer, "bridge", name, ifname,
- script, downscript, tap->vhostfd,
+ ret = net_init_tap_one(tap, peer, "bridge", name, NULL,
+ NULL, NULL, tap->vhostfd,
vnet_hdr, fd, errp);
if (ret < 0) {
close(fd);
return -1;
}
} else {
+ const char *script = tap->script;
+ const char *downscript = tap->downscript;
+ int queues = tap->has_queues ? tap->queues : 1;
g_autofree char *default_script = NULL;
g_autofree char *default_downscript = NULL;
+ char ifname[128];
+
if (tap->vhostfds) {
error_setg(errp, "vhostfds= is invalid if fds= wasn't specified");
return -1;
--
2.48.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2 09/20] net/tap: use glib strings vector and g_strsplit for fds case
2025-08-23 16:03 [PATCH v2 00/20] TAP initialization refactoring Vladimir Sementsov-Ogievskiy
` (7 preceding siblings ...)
2025-08-23 16:03 ` [PATCH v2 08/20] net/tap: move local variables related to the latter case to else branch Vladimir Sementsov-Ogievskiy
@ 2025-08-23 16:03 ` Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 10/20] net/tap: drop extra tap_fd_get_ifname() call Vladimir Sementsov-Ogievskiy
` (12 subsequent siblings)
21 siblings, 0 replies; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-08-23 16:03 UTC (permalink / raw)
To: jasowang; +Cc: qemu-devel, vsementsov, leiyang, steven.sistare
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
net/tap.c | 72 +++++++++++--------------------------------------------
1 file changed, 14 insertions(+), 58 deletions(-)
diff --git a/net/tap.c b/net/tap.c
index 80ec54f914..ac8d955050 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -774,32 +774,6 @@ failed:
return -1;
}
-static int get_fds(char *str, char *fds[], int max)
-{
- char *ptr = str, *this;
- size_t len = strlen(str);
- int i = 0;
-
- while (i < max && ptr < str + len) {
- this = strchr(ptr, ':');
-
- if (this == NULL) {
- fds[i] = g_strdup(ptr);
- } else {
- fds[i] = g_strndup(ptr, this - ptr);
- }
-
- i++;
- if (this == NULL) {
- break;
- } else {
- ptr = this + 1;
- }
- }
-
- return i;
-}
-
int net_init_tap(const Netdev *netdev, const char *name,
NetClientState *peer, Error **errp)
{
@@ -859,74 +833,56 @@ int net_init_tap(const Netdev *netdev, const char *name,
return -1;
}
} else if (tap->fds) {
- char **fds;
- char **vhost_fds;
- int nfds = 0, nvhosts = 0;
+ g_auto(GStrv) fds = NULL;
+ g_auto(GStrv) vhost_fds = NULL;
+ int nfds;
if (tap->helper || tap->vhostfd) {
error_setg(errp, "helper= and vhostfd= are invalid with fds=");
return -1;
}
- fds = g_new0(char *, MAX_TAP_QUEUES);
- vhost_fds = g_new0(char *, MAX_TAP_QUEUES);
+ fds = g_strsplit(tap->fds, ":", MAX_TAP_QUEUES);
+ nfds = g_strv_length(fds);
- nfds = get_fds(tap->fds, fds, MAX_TAP_QUEUES);
if (tap->vhostfds) {
- nvhosts = get_fds(tap->vhostfds, vhost_fds, MAX_TAP_QUEUES);
- if (nfds != nvhosts) {
+ vhost_fds = g_strsplit(tap->vhostfds, ":", MAX_TAP_QUEUES);
+ if (nfds != g_strv_length(vhost_fds)) {
error_setg(errp, "The number of fds passed does not match "
"the number of vhostfds passed");
- ret = -1;
- goto free_fail;
+ return -1;
}
}
for (i = 0; i < nfds; i++) {
fd = monitor_fd_param(monitor_cur(), fds[i], errp);
if (fd == -1) {
- ret = -1;
- goto free_fail;
+ return -1;
}
if (!set_fd_nonblocking(fd, fds[i], errp)) {
- ret = -1;
- goto free_fail;
+ return -1;
}
if (i == 0) {
vnet_hdr = tap_probe_vnet_hdr(fd, errp);
if (vnet_hdr < 0) {
- ret = -1;
- goto free_fail;
+ return -1;
}
} else if (vnet_hdr != tap_probe_vnet_hdr(fd, NULL)) {
error_setg(errp,
"vnet_hdr not consistent across given tap fds");
- ret = -1;
- goto free_fail;
+ return -1;
}
ret = net_init_tap_one(tap, peer, "tap", name, NULL,
NULL, NULL,
- tap->vhostfds ? vhost_fds[i] : NULL,
+ vhost_fds ? vhost_fds[i] : NULL,
vnet_hdr, fd, errp);
if (ret < 0) {
- ret = -1;
- goto free_fail;
+ return -1;
}
}
-
-free_fail:
- for (i = 0; i < nvhosts; i++) {
- g_free(vhost_fds[i]);
- }
- for (i = 0; i < nfds; i++) {
- g_free(fds[i]);
- }
- g_free(fds);
- g_free(vhost_fds);
- return ret;
} else if (tap->helper) {
if (tap->vhostfds) {
error_setg(errp, "vhostfds= is invalid with helper=");
--
2.48.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2 10/20] net/tap: drop extra tap_fd_get_ifname() call
2025-08-23 16:03 [PATCH v2 00/20] TAP initialization refactoring Vladimir Sementsov-Ogievskiy
` (8 preceding siblings ...)
2025-08-23 16:03 ` [PATCH v2 09/20] net/tap: use glib strings vector and g_strsplit for fds case Vladimir Sementsov-Ogievskiy
@ 2025-08-23 16:03 ` Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 11/20] net/tap: net_init_tap_one(): refactor to use netdev as first arg Vladimir Sementsov-Ogievskiy
` (11 subsequent siblings)
21 siblings, 0 replies; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-08-23 16:03 UTC (permalink / raw)
To: jasowang; +Cc: qemu-devel, vsementsov, leiyang, steven.sistare
tap_open() cares to update ifname, no reason to call extra ioctl.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
net/tap.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/net/tap.c b/net/tap.c
index ac8d955050..eab0a86173 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -946,14 +946,6 @@ int net_init_tap(const Netdev *netdev, const char *name,
return -1;
}
- if (queues > 1 && i == 0 && !tap->ifname) {
- if (tap_fd_get_ifname(fd, ifname)) {
- error_setg(errp, "Fail to get ifname");
- close(fd);
- return -1;
- }
- }
-
ret = net_init_tap_one(tap, peer, "tap", name, ifname,
i >= 1 ? "no" : script,
i >= 1 ? "no" : downscript,
--
2.48.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2 11/20] net/tap: net_init_tap_one(): refactor to use netdev as first arg
2025-08-23 16:03 [PATCH v2 00/20] TAP initialization refactoring Vladimir Sementsov-Ogievskiy
` (9 preceding siblings ...)
2025-08-23 16:03 ` [PATCH v2 10/20] net/tap: drop extra tap_fd_get_ifname() call Vladimir Sementsov-Ogievskiy
@ 2025-08-23 16:03 ` Vladimir Sementsov-Ogievskiy
2025-09-03 5:19 ` Jason Wang
2025-08-23 16:03 ` [PATCH v2 12/20] net/tap: net_init_tap_one(): support bridge Vladimir Sementsov-Ogievskiy
` (10 subsequent siblings)
21 siblings, 1 reply; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-08-23 16:03 UTC (permalink / raw)
To: jasowang; +Cc: qemu-devel, vsementsov, leiyang, steven.sistare
This is needed for further unification of bridge initialization in
net_init_tap() and net_init_bridge().
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
net/tap.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/net/tap.c b/net/tap.c
index eab0a86173..468dae7004 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -692,15 +692,18 @@ static int net_tap_init(const NetdevTapOptions *tap, int *vnet_hdr,
#define MAX_TAP_QUEUES 1024
-static int net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
+static int net_init_tap_one(const Netdev *netdev, NetClientState *peer,
const char *model, const char *name,
const char *ifname, const char *script,
const char *downscript, const char *vhostfdname,
int vnet_hdr, int fd, Error **errp)
{
+ const NetdevTapOptions *tap = &netdev->u.tap;
TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
int vhostfd;
+ assert(netdev->type == NET_CLIENT_DRIVER_TAP);
+
if (tap_set_sndbuf(s->fd, tap, errp) < 0) {
goto failed;
}
@@ -826,7 +829,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
return -1;
}
- ret = net_init_tap_one(tap, peer, "tap", name, NULL,
+ ret = net_init_tap_one(netdev, peer, "tap", name, NULL,
NULL, NULL,
tap->vhostfd, vnet_hdr, fd, errp);
if (ret < 0) {
@@ -875,7 +878,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
return -1;
}
- ret = net_init_tap_one(tap, peer, "tap", name, NULL,
+ ret = net_init_tap_one(netdev, peer, "tap", name, NULL,
NULL, NULL,
vhost_fds ? vhost_fds[i] : NULL,
vnet_hdr, fd, errp);
@@ -905,7 +908,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
return -1;
}
- ret = net_init_tap_one(tap, peer, "bridge", name, NULL,
+ ret = net_init_tap_one(netdev, peer, "bridge", name, NULL,
NULL, NULL, tap->vhostfd,
vnet_hdr, fd, errp);
if (ret < 0) {
@@ -946,7 +949,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
return -1;
}
- ret = net_init_tap_one(tap, peer, "tap", name, ifname,
+ ret = net_init_tap_one(netdev, peer, "tap", name, ifname,
i >= 1 ? "no" : script,
i >= 1 ? "no" : downscript,
tap->vhostfd, vnet_hdr, fd, errp);
--
2.48.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2 12/20] net/tap: net_init_tap_one(): support bridge
2025-08-23 16:03 [PATCH v2 00/20] TAP initialization refactoring Vladimir Sementsov-Ogievskiy
` (10 preceding siblings ...)
2025-08-23 16:03 ` [PATCH v2 11/20] net/tap: net_init_tap_one(): refactor to use netdev as first arg Vladimir Sementsov-Ogievskiy
@ 2025-08-23 16:03 ` Vladimir Sementsov-Ogievskiy
2025-09-03 5:28 ` Jason Wang
2025-08-23 16:03 ` [PATCH v2 13/20] net/tap: net_init_bridge(): support tap Vladimir Sementsov-Ogievskiy
` (9 subsequent siblings)
21 siblings, 1 reply; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-08-23 16:03 UTC (permalink / raw)
To: jasowang; +Cc: qemu-devel, vsementsov, leiyang, steven.sistare
Use net_init_tap_one() in net_init_bridge().
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
net/tap.c | 28 +++++++++++++++++++++++-----
1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/net/tap.c b/net/tap.c
index 468dae7004..5acfb128a1 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -88,6 +88,12 @@ static void launch_script(const char *setup_script, const char *ifname,
static void tap_send(void *opaque);
static void tap_writable(void *opaque);
+static int net_init_tap_one(const Netdev *netdev, NetClientState *peer,
+ const char *model, const char *name,
+ const char *ifname, const char *script,
+ const char *downscript, const char *vhostfdname,
+ int vnet_hdr, int fd, Error **errp);
+
static void tap_update_fd_handler(TAPState *s)
{
qemu_set_fd_handler(s->fd,
@@ -626,8 +632,7 @@ int net_init_bridge(const Netdev *netdev, const char *name,
{
const NetdevBridgeOptions *bridge;
const char *helper, *br;
- TAPState *s;
- int fd, vnet_hdr;
+ int fd, vnet_hdr, ret;
assert(netdev->type == NET_CLIENT_DRIVER_BRIDGE);
bridge = &netdev->u.bridge;
@@ -648,9 +653,14 @@ int net_init_bridge(const Netdev *netdev, const char *name,
close(fd);
return -1;
}
- s = net_tap_fd_init(peer, "bridge", name, fd, vnet_hdr);
- qemu_set_info_str(&s->nc, "helper=%s,br=%s", helper, br);
+ ret = net_init_tap_one(netdev, peer, "bridge", name,
+ NULL, NULL, NULL,
+ NULL, vnet_hdr, fd, errp);
+ if (ret < 0) {
+ close(fd);
+ return -1;
+ }
return 0;
}
@@ -698,11 +708,19 @@ static int net_init_tap_one(const Netdev *netdev, NetClientState *peer,
const char *downscript, const char *vhostfdname,
int vnet_hdr, int fd, Error **errp)
{
- const NetdevTapOptions *tap = &netdev->u.tap;
+ const NetdevTapOptions *tap;
TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
int vhostfd;
+ if (netdev->type == NET_CLIENT_DRIVER_BRIDGE) {
+ const NetdevBridgeOptions *bridge = &netdev->u.bridge;
+ qemu_set_info_str(&s->nc, "helper=%s,br=%s",
+ bridge->helper, bridge->br);
+ return 0;
+ }
+
assert(netdev->type == NET_CLIENT_DRIVER_TAP);
+ tap = &netdev->u.tap;
if (tap_set_sndbuf(s->fd, tap, errp) < 0) {
goto failed;
--
2.48.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2 13/20] net/tap: net_init_bridge(): support tap
2025-08-23 16:03 [PATCH v2 00/20] TAP initialization refactoring Vladimir Sementsov-Ogievskiy
` (11 preceding siblings ...)
2025-08-23 16:03 ` [PATCH v2 12/20] net/tap: net_init_tap_one(): support bridge Vladimir Sementsov-Ogievskiy
@ 2025-08-23 16:03 ` Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 14/20] net/tap: refactor net_tap_init() into net_tap_open_one() Vladimir Sementsov-Ogievskiy
` (8 subsequent siblings)
21 siblings, 0 replies; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-08-23 16:03 UTC (permalink / raw)
To: jasowang; +Cc: qemu-devel, vsementsov, leiyang, steven.sistare
Support tap in net_init_bridge() and reuse it for corresponding
case in net_init_tap().
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
net/tap.c | 47 +++++++++++++++++------------------------------
1 file changed, 17 insertions(+), 30 deletions(-)
diff --git a/net/tap.c b/net/tap.c
index 5acfb128a1..83a1c9250a 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -630,16 +630,24 @@ static bool set_fd_nonblocking(int fd, const char *note, Error **errp)
int net_init_bridge(const Netdev *netdev, const char *name,
NetClientState *peer, Error **errp)
{
- const NetdevBridgeOptions *bridge;
+ const NetdevTapOptions *tap = NULL;
+ const NetdevBridgeOptions *bridge = NULL;
const char *helper, *br;
int fd, vnet_hdr, ret;
- assert(netdev->type == NET_CLIENT_DRIVER_BRIDGE);
- bridge = &netdev->u.bridge;
- helper = bridge->helper;
- br = bridge->br ?: DEFAULT_BRIDGE_INTERFACE;
+ if (netdev->type == NET_CLIENT_DRIVER_BRIDGE) {
+ bridge = &netdev->u.bridge;
+ helper = bridge->helper;
+ br = bridge->br;
+ } else {
+ assert(netdev->type == NET_CLIENT_DRIVER_TAP);
- fd = net_bridge_run_helper(helper, br, errp);
+ tap = &netdev->u.tap;
+ helper = tap->helper;
+ br = tap->br;
+ }
+
+ fd = net_bridge_run_helper(helper, br ?: DEFAULT_BRIDGE_INTERFACE, errp);
if (fd == -1) {
return -1;
}
@@ -656,7 +664,8 @@ int net_init_bridge(const Netdev *netdev, const char *name,
ret = net_init_tap_one(netdev, peer, "bridge", name,
NULL, NULL, NULL,
- NULL, vnet_hdr, fd, errp);
+ tap ? tap->vhostfd : NULL,
+ vnet_hdr, fd, errp);
if (ret < 0) {
close(fd);
return -1;
@@ -910,29 +919,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
return -1;
}
- fd = net_bridge_run_helper(tap->helper,
- tap->br ?: DEFAULT_BRIDGE_INTERFACE,
- errp);
- if (fd == -1) {
- return -1;
- }
-
- if (!set_fd_nonblocking(fd, name, errp)) {
- return -1;
- }
- vnet_hdr = tap_probe_vnet_hdr(fd, errp);
- if (vnet_hdr < 0) {
- close(fd);
- return -1;
- }
-
- ret = net_init_tap_one(netdev, peer, "bridge", name, NULL,
- NULL, NULL, tap->vhostfd,
- vnet_hdr, fd, errp);
- if (ret < 0) {
- close(fd);
- return -1;
- }
+ return net_init_bridge(netdev, name, peer, errp);
} else {
const char *script = tap->script;
const char *downscript = tap->downscript;
--
2.48.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2 14/20] net/tap: refactor net_tap_init() into net_tap_open_one()
2025-08-23 16:03 [PATCH v2 00/20] TAP initialization refactoring Vladimir Sementsov-Ogievskiy
` (12 preceding siblings ...)
2025-08-23 16:03 ` [PATCH v2 13/20] net/tap: net_init_bridge(): support tap Vladimir Sementsov-Ogievskiy
@ 2025-08-23 16:03 ` Vladimir Sementsov-Ogievskiy
2025-09-03 5:36 ` Jason Wang
2025-08-23 16:03 ` [PATCH v2 15/20] net/tap: introduce net_tap_open() Vladimir Sementsov-Ogievskiy
` (7 subsequent siblings)
21 siblings, 1 reply; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-08-23 16:03 UTC (permalink / raw)
To: jasowang; +Cc: qemu-devel, vsementsov, leiyang, steven.sistare
net_tap_init() is used in one place. Let's move net_init_tap_one()
call to it and simplify outer loop code.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
net/tap.c | 54 +++++++++++++++++++++++++++++++-----------------------
1 file changed, 31 insertions(+), 23 deletions(-)
diff --git a/net/tap.c b/net/tap.c
index 83a1c9250a..57939ed16f 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -674,31 +674,37 @@ int net_init_bridge(const Netdev *netdev, const char *name,
return 0;
}
-static int net_tap_init(const NetdevTapOptions *tap, int *vnet_hdr,
- const char *setup_script, char *ifname,
- size_t ifname_sz, int mq_required, Error **errp)
+static int net_tap_open_one(const Netdev *netdev,
+ const char *name, NetClientState *peer,
+ const char *script, const char *downscript,
+ char *ifname, size_t ifname_sz,
+ int mq_required, Error **errp)
{
+ const NetdevTapOptions *tap = &netdev->u.tap;
Error *err = NULL;
- int fd, vnet_hdr_required;
+ int fd, vnet_hdr_required, vnet_hdr;
+ int ret;
+
+ assert(netdev->type == NET_CLIENT_DRIVER_TAP);
if (tap->has_vnet_hdr) {
- *vnet_hdr = tap->vnet_hdr;
- vnet_hdr_required = *vnet_hdr;
+ vnet_hdr = tap->vnet_hdr;
+ vnet_hdr_required = vnet_hdr;
} else {
- *vnet_hdr = 1;
+ vnet_hdr = 1;
vnet_hdr_required = 0;
}
- fd = RETRY_ON_EINTR(tap_open(ifname, ifname_sz, vnet_hdr, vnet_hdr_required,
- mq_required, errp));
+ fd = RETRY_ON_EINTR(tap_open(ifname, ifname_sz, &vnet_hdr,
+ vnet_hdr_required, mq_required, errp));
if (fd < 0) {
return -1;
}
- if (setup_script &&
- setup_script[0] != '\0' &&
- strcmp(setup_script, "no") != 0) {
- launch_script(setup_script, ifname, fd, &err);
+ if (script &&
+ script[0] != '\0' &&
+ strcmp(script, "no") != 0) {
+ launch_script(script, ifname, fd, &err);
if (err) {
error_propagate(errp, err);
close(fd);
@@ -706,7 +712,15 @@ static int net_tap_init(const NetdevTapOptions *tap, int *vnet_hdr,
}
}
- return fd;
+ ret = net_init_tap_one(netdev, peer, "tap", name, ifname,
+ script, downscript,
+ tap->vhostfd, vnet_hdr, fd, errp);
+ if (ret < 0) {
+ close(fd);
+ return -1;
+ }
+
+ return 0;
}
#define MAX_TAP_QUEUES 1024
@@ -948,20 +962,14 @@ int net_init_tap(const Netdev *netdev, const char *name,
}
for (i = 0; i < queues; i++) {
- fd = net_tap_init(tap, &vnet_hdr, i >= 1 ? "no" : script,
- ifname, sizeof ifname, queues > 1, errp);
- if (fd == -1) {
- return -1;
- }
-
- ret = net_init_tap_one(netdev, peer, "tap", name, ifname,
+ ret = net_tap_open_one(netdev, name, peer,
i >= 1 ? "no" : script,
i >= 1 ? "no" : downscript,
- tap->vhostfd, vnet_hdr, fd, errp);
+ ifname, sizeof ifname, queues > 1, errp);
if (ret < 0) {
- close(fd);
return -1;
}
+
}
}
--
2.48.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2 15/20] net/tap: introduce net_tap_open()
2025-08-23 16:03 [PATCH v2 00/20] TAP initialization refactoring Vladimir Sementsov-Ogievskiy
` (13 preceding siblings ...)
2025-08-23 16:03 ` [PATCH v2 14/20] net/tap: refactor net_tap_init() into net_tap_open_one() Vladimir Sementsov-Ogievskiy
@ 2025-08-23 16:03 ` Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 16/20] net/tap: introduce net_tap_fd_init_external() Vladimir Sementsov-Ogievskiy
` (6 subsequent siblings)
21 siblings, 0 replies; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-08-23 16:03 UTC (permalink / raw)
To: jasowang; +Cc: qemu-devel, vsementsov, leiyang, steven.sistare
Move the latter case of net_init_tap() to a separate function.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
net/tap.c | 96 ++++++++++++++++++++++++++++++-------------------------
1 file changed, 53 insertions(+), 43 deletions(-)
diff --git a/net/tap.c b/net/tap.c
index 57939ed16f..27642c45a9 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -818,6 +818,49 @@ failed:
return -1;
}
+static int net_tap_open(const Netdev *netdev,
+ const char *name,
+ NetClientState *peer,
+ Error **errp)
+{
+ const NetdevTapOptions *tap = &netdev->u.tap;
+ const char *script = tap->script;
+ const char *downscript = tap->downscript;
+ int queues = tap->has_queues ? tap->queues : 1;
+ g_autofree char *default_script = NULL;
+ g_autofree char *default_downscript = NULL;
+ char ifname[128];
+ int i, ret;
+
+ assert(netdev->type == NET_CLIENT_DRIVER_TAP);
+
+ if (!script) {
+ script = default_script = get_relocated_path(DEFAULT_NETWORK_SCRIPT);
+ }
+ if (!downscript) {
+ downscript = default_downscript =
+ get_relocated_path(DEFAULT_NETWORK_DOWN_SCRIPT);
+ }
+
+ if (tap->ifname) {
+ pstrcpy(ifname, sizeof ifname, tap->ifname);
+ } else {
+ ifname[0] = '\0';
+ }
+
+ for (i = 0; i < queues; i++) {
+ ret = net_tap_open_one(netdev, name, peer,
+ i >= 1 ? "no" : script,
+ i >= 1 ? "no" : downscript,
+ ifname, sizeof ifname, queues > 1, errp);
+ if (ret < 0) {
+ return -1;
+ }
+ }
+
+ return 0;
+}
+
int net_init_tap(const Netdev *netdev, const char *name,
NetClientState *peer, Error **errp)
{
@@ -870,12 +913,9 @@ int net_init_tap(const Netdev *netdev, const char *name,
return -1;
}
- ret = net_init_tap_one(netdev, peer, "tap", name, NULL,
- NULL, NULL,
- tap->vhostfd, vnet_hdr, fd, errp);
- if (ret < 0) {
- return -1;
- }
+ return net_init_tap_one(netdev, peer, "tap", name, NULL,
+ NULL, NULL,
+ tap->vhostfd, vnet_hdr, fd, errp);
} else if (tap->fds) {
g_auto(GStrv) fds = NULL;
g_auto(GStrv) vhost_fds = NULL;
@@ -927,6 +967,8 @@ int net_init_tap(const Netdev *netdev, const char *name,
return -1;
}
}
+
+ return 0;
} else if (tap->helper) {
if (tap->vhostfds) {
error_setg(errp, "vhostfds= is invalid with helper=");
@@ -934,46 +976,14 @@ int net_init_tap(const Netdev *netdev, const char *name,
}
return net_init_bridge(netdev, name, peer, errp);
- } else {
- const char *script = tap->script;
- const char *downscript = tap->downscript;
- int queues = tap->has_queues ? tap->queues : 1;
- g_autofree char *default_script = NULL;
- g_autofree char *default_downscript = NULL;
- char ifname[128];
-
- if (tap->vhostfds) {
- error_setg(errp, "vhostfds= is invalid if fds= wasn't specified");
- return -1;
- }
-
- if (!script) {
- script = default_script = get_relocated_path(DEFAULT_NETWORK_SCRIPT);
- }
- if (!downscript) {
- downscript = default_downscript =
- get_relocated_path(DEFAULT_NETWORK_DOWN_SCRIPT);
- }
-
- if (tap->ifname) {
- pstrcpy(ifname, sizeof ifname, tap->ifname);
- } else {
- ifname[0] = '\0';
- }
-
- for (i = 0; i < queues; i++) {
- ret = net_tap_open_one(netdev, name, peer,
- i >= 1 ? "no" : script,
- i >= 1 ? "no" : downscript,
- ifname, sizeof ifname, queues > 1, errp);
- if (ret < 0) {
- return -1;
- }
+ }
- }
+ if (tap->vhostfds) {
+ error_setg(errp, "vhostfds= is invalid if fds= wasn't specified");
+ return -1;
}
- return 0;
+ return net_tap_open(netdev, name, peer, errp);
}
int tap_enable(NetClientState *nc)
--
2.48.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2 16/20] net/tap: introduce net_tap_fd_init_external()
2025-08-23 16:03 [PATCH v2 00/20] TAP initialization refactoring Vladimir Sementsov-Ogievskiy
` (14 preceding siblings ...)
2025-08-23 16:03 ` [PATCH v2 15/20] net/tap: introduce net_tap_open() Vladimir Sementsov-Ogievskiy
@ 2025-08-23 16:03 ` Vladimir Sementsov-Ogievskiy
2025-09-03 5:37 ` Jason Wang
2025-08-23 16:03 ` [PATCH v2 17/20] net/tap: introduce net_tap_from_monitor_fd() helper Vladimir Sementsov-Ogievskiy
` (5 subsequent siblings)
21 siblings, 1 reply; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-08-23 16:03 UTC (permalink / raw)
To: jasowang; +Cc: qemu-devel, vsementsov, leiyang, steven.sistare
Add helper that covers logic for initializing fds, given from monitor
or helper.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
net/tap.c | 90 ++++++++++++++++++++++---------------------------------
1 file changed, 36 insertions(+), 54 deletions(-)
diff --git a/net/tap.c b/net/tap.c
index 27642c45a9..8cea6ed87b 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -627,13 +627,40 @@ static bool set_fd_nonblocking(int fd, const char *note, Error **errp)
return ok;
}
+static int net_tap_fd_init_external(const Netdev *netdev, NetClientState *peer,
+ const char *model, const char *name,
+ const char *vhostfdname,
+ int *pvnet_hdr, int fd, Error **errp)
+{
+ int vnet_hdr;
+
+ if (!set_fd_nonblocking(fd, name, errp)) {
+ return -1;
+ }
+
+ vnet_hdr = tap_probe_vnet_hdr(fd, errp);
+ if (pvnet_hdr) {
+ if (*pvnet_hdr < 0) {
+ *pvnet_hdr = vnet_hdr;
+ } else if (vnet_hdr != *pvnet_hdr) {
+ error_setg(errp,
+ "vnet_hdr not consistent across given tap fds");
+ return -1;
+ }
+ }
+
+ return net_init_tap_one(netdev, peer, model, name,
+ NULL, NULL, NULL,
+ vhostfdname, vnet_hdr, fd, errp);
+}
+
int net_init_bridge(const Netdev *netdev, const char *name,
NetClientState *peer, Error **errp)
{
const NetdevTapOptions *tap = NULL;
const NetdevBridgeOptions *bridge = NULL;
const char *helper, *br;
- int fd, vnet_hdr, ret;
+ int fd;
if (netdev->type == NET_CLIENT_DRIVER_BRIDGE) {
bridge = &netdev->u.bridge;
@@ -652,26 +679,8 @@ int net_init_bridge(const Netdev *netdev, const char *name,
return -1;
}
- if (!set_fd_nonblocking(fd, name, errp)) {
- return -1;
- }
-
- vnet_hdr = tap_probe_vnet_hdr(fd, errp);
- if (vnet_hdr < 0) {
- close(fd);
- return -1;
- }
-
- ret = net_init_tap_one(netdev, peer, "bridge", name,
- NULL, NULL, NULL,
- tap ? tap->vhostfd : NULL,
- vnet_hdr, fd, errp);
- if (ret < 0) {
- close(fd);
- return -1;
- }
-
- return 0;
+ return net_tap_fd_init_external(netdev, peer, "bridge", name,
+ tap ? tap->vhostfd : NULL, NULL, fd, errp);
}
static int net_tap_open_one(const Netdev *netdev,
@@ -902,20 +911,8 @@ int net_init_tap(const Netdev *netdev, const char *name,
return -1;
}
- if (!set_fd_nonblocking(fd, tap->fd, errp)) {
- close(fd);
- return -1;
- }
-
- vnet_hdr = tap_probe_vnet_hdr(fd, errp);
- if (vnet_hdr < 0) {
- close(fd);
- return -1;
- }
-
- return net_init_tap_one(netdev, peer, "tap", name, NULL,
- NULL, NULL,
- tap->vhostfd, vnet_hdr, fd, errp);
+ return net_tap_fd_init_external(netdev, peer, "tap", name,
+ tap->vhostfd, NULL, fd, errp);
} else if (tap->fds) {
g_auto(GStrv) fds = NULL;
g_auto(GStrv) vhost_fds = NULL;
@@ -938,31 +935,16 @@ int net_init_tap(const Netdev *netdev, const char *name,
}
}
+ vnet_hdr = -1;
for (i = 0; i < nfds; i++) {
fd = monitor_fd_param(monitor_cur(), fds[i], errp);
if (fd == -1) {
return -1;
}
- if (!set_fd_nonblocking(fd, fds[i], errp)) {
- return -1;
- }
-
- if (i == 0) {
- vnet_hdr = tap_probe_vnet_hdr(fd, errp);
- if (vnet_hdr < 0) {
- return -1;
- }
- } else if (vnet_hdr != tap_probe_vnet_hdr(fd, NULL)) {
- error_setg(errp,
- "vnet_hdr not consistent across given tap fds");
- return -1;
- }
-
- ret = net_init_tap_one(netdev, peer, "tap", name, NULL,
- NULL, NULL,
- vhost_fds ? vhost_fds[i] : NULL,
- vnet_hdr, fd, errp);
+ ret = net_tap_fd_init_external(netdev, peer, "tap", name,
+ vhost_fds ? vhost_fds[i] : NULL,
+ &vnet_hdr, fd, errp);
if (ret < 0) {
return -1;
}
--
2.48.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2 17/20] net/tap: introduce net_tap_from_monitor_fd() helper
2025-08-23 16:03 [PATCH v2 00/20] TAP initialization refactoring Vladimir Sementsov-Ogievskiy
` (15 preceding siblings ...)
2025-08-23 16:03 ` [PATCH v2 16/20] net/tap: introduce net_tap_fd_init_external() Vladimir Sementsov-Ogievskiy
@ 2025-08-23 16:03 ` Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 18/20] net/tap: split net_tap_setup_vhost() separate function Vladimir Sementsov-Ogievskiy
` (4 subsequent siblings)
21 siblings, 0 replies; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-08-23 16:03 UTC (permalink / raw)
To: jasowang; +Cc: qemu-devel, vsementsov, leiyang, steven.sistare
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
net/tap.c | 34 +++++++++++++++++++---------------
1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/net/tap.c b/net/tap.c
index 8cea6ed87b..2ef1db0ff1 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -827,6 +827,20 @@ failed:
return -1;
}
+static int net_tap_from_monitor_fd(const Netdev *netdev, NetClientState *peer,
+ const char *name, const char *vhostfdname,
+ int *pvnet_hdr, const char *fdname,
+ Error **errp)
+{
+ int fd = monitor_fd_param(monitor_cur(), fdname, errp);
+ if (fd == -1) {
+ return -1;
+ }
+
+ return net_tap_fd_init_external(netdev, peer, "tap", name,
+ vhostfdname, pvnet_hdr, fd, errp);
+}
+
static int net_tap_open(const Netdev *netdev,
const char *name,
NetClientState *peer,
@@ -874,7 +888,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
NetClientState *peer, Error **errp)
{
const NetdevTapOptions *tap = &netdev->u.tap;
- int fd, vnet_hdr = 0, i = 0;
+ int vnet_hdr = 0, i = 0;
int ret = 0;
assert(netdev->type == NET_CLIENT_DRIVER_TAP);
@@ -906,13 +920,8 @@ int net_init_tap(const Netdev *netdev, const char *name,
return -1;
}
- fd = monitor_fd_param(monitor_cur(), tap->fd, errp);
- if (fd == -1) {
- return -1;
- }
-
- return net_tap_fd_init_external(netdev, peer, "tap", name,
- tap->vhostfd, NULL, fd, errp);
+ return net_tap_from_monitor_fd(netdev, peer, name, tap->vhostfd,
+ NULL, tap->fd, errp);
} else if (tap->fds) {
g_auto(GStrv) fds = NULL;
g_auto(GStrv) vhost_fds = NULL;
@@ -937,14 +946,9 @@ int net_init_tap(const Netdev *netdev, const char *name,
vnet_hdr = -1;
for (i = 0; i < nfds; i++) {
- fd = monitor_fd_param(monitor_cur(), fds[i], errp);
- if (fd == -1) {
- return -1;
- }
-
- ret = net_tap_fd_init_external(netdev, peer, "tap", name,
+ ret = net_tap_from_monitor_fd(netdev, peer, name,
vhost_fds ? vhost_fds[i] : NULL,
- &vnet_hdr, fd, errp);
+ &vnet_hdr, fds[i], errp);
if (ret < 0) {
return -1;
}
--
2.48.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2 18/20] net/tap: split net_tap_setup_vhost() separate function
2025-08-23 16:03 [PATCH v2 00/20] TAP initialization refactoring Vladimir Sementsov-Ogievskiy
` (16 preceding siblings ...)
2025-08-23 16:03 ` [PATCH v2 17/20] net/tap: introduce net_tap_from_monitor_fd() helper Vladimir Sementsov-Ogievskiy
@ 2025-08-23 16:03 ` Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 19/20] net/tap: drop net_tap_fd_init() Vladimir Sementsov-Ogievskiy
` (3 subsequent siblings)
21 siblings, 0 replies; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-08-23 16:03 UTC (permalink / raw)
To: jasowang; +Cc: qemu-devel, vsementsov, leiyang, steven.sistare
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
net/tap.c | 34 ++++++++++++++++++++++++----------
1 file changed, 24 insertions(+), 10 deletions(-)
diff --git a/net/tap.c b/net/tap.c
index 2ef1db0ff1..f2c0f7fd06 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -93,6 +93,8 @@ static int net_init_tap_one(const Netdev *netdev, NetClientState *peer,
const char *ifname, const char *script,
const char *downscript, const char *vhostfdname,
int vnet_hdr, int fd, Error **errp);
+static int net_tap_setup_vhost(TAPState *s, const NetdevTapOptions *tap,
+ const char *vhostfdname, Error **errp);
static void tap_update_fd_handler(TAPState *s)
{
@@ -742,7 +744,7 @@ static int net_init_tap_one(const Netdev *netdev, NetClientState *peer,
{
const NetdevTapOptions *tap;
TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
- int vhostfd;
+ int ret;
if (netdev->type == NET_CLIENT_DRIVER_BRIDGE) {
const NetdevBridgeOptions *bridge = &netdev->u.bridge;
@@ -773,9 +775,25 @@ static int net_init_tap_one(const Netdev *netdev, NetClientState *peer,
}
}
+ ret = net_tap_setup_vhost(s, tap, vhostfdname, errp);
+ if (ret < 0) {
+ goto failed;
+ }
+
+ return 0;
+
+failed:
+ qemu_del_net_client(&s->nc);
+ return -1;
+}
+
+static int net_tap_setup_vhost(TAPState *s, const NetdevTapOptions *tap,
+ const char *vhostfdname, Error **errp)
+{
if (tap->has_vhost ? tap->vhost :
vhostfdname || (tap->has_vhostforce && tap->vhostforce)) {
VhostNetOptions options;
+ int vhostfd;
options.backend_type = VHOST_BACKEND_TYPE_KERNEL;
options.net_backend = &s->nc;
@@ -788,20 +806,20 @@ static int net_init_tap_one(const Netdev *netdev, NetClientState *peer,
if (vhostfdname) {
vhostfd = monitor_fd_param(monitor_cur(), vhostfdname, errp);
if (vhostfd == -1) {
- goto failed;
+ return -1;
}
if (!set_fd_nonblocking(vhostfd, vhostfdname, errp)) {
- goto failed;
+ return -1;
}
} else {
vhostfd = open("/dev/vhost-net", O_RDWR);
if (vhostfd < 0) {
error_setg_errno(errp, errno,
"tap: open vhost char device failed");
- goto failed;
+ return -1;
}
if (!set_fd_nonblocking(vhostfd, "opened /dev/vhost-net", errp)) {
- goto failed;
+ return -1;
}
}
options.opaque = (void *)(uintptr_t)vhostfd;
@@ -816,15 +834,11 @@ static int net_init_tap_one(const Netdev *netdev, NetClientState *peer,
if (!s->vhost_net) {
error_setg(errp,
"vhost-net requested but could not be initialized");
- goto failed;
+ return -1;
}
}
return 0;
-
-failed:
- qemu_del_net_client(&s->nc);
- return -1;
}
static int net_tap_from_monitor_fd(const Netdev *netdev, NetClientState *peer,
--
2.48.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2 19/20] net/tap: drop net_tap_fd_init()
2025-08-23 16:03 [PATCH v2 00/20] TAP initialization refactoring Vladimir Sementsov-Ogievskiy
` (17 preceding siblings ...)
2025-08-23 16:03 ` [PATCH v2 18/20] net/tap: split net_tap_setup_vhost() separate function Vladimir Sementsov-Ogievskiy
@ 2025-08-23 16:03 ` Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 20/20] net/tap: introduce net_init_tap_fds() Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
21 siblings, 0 replies; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-08-23 16:03 UTC (permalink / raw)
To: jasowang; +Cc: qemu-devel, vsementsov, leiyang, steven.sistare
It's called only from net_init_tap_one(), and there is no semantic
reason, why we do part of initialization in one function and continue
in another. Let's combine them into one net_tap_fd_init_common().
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
net/tap.c | 97 +++++++++++++++++++++++++------------------------------
1 file changed, 44 insertions(+), 53 deletions(-)
diff --git a/net/tap.c b/net/tap.c
index f2c0f7fd06..08927088eb 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -88,11 +88,12 @@ static void launch_script(const char *setup_script, const char *ifname,
static void tap_send(void *opaque);
static void tap_writable(void *opaque);
-static int net_init_tap_one(const Netdev *netdev, NetClientState *peer,
- const char *model, const char *name,
- const char *ifname, const char *script,
- const char *downscript, const char *vhostfdname,
- int vnet_hdr, int fd, Error **errp);
+static int net_tap_fd_init_common(const Netdev *netdev, NetClientState *peer,
+ const char *model, const char *name,
+ const char *ifname, const char *script,
+ const char *downscript,
+ const char *vhostfdname,
+ int vnet_hdr, int fd, Error **errp);
static int net_tap_setup_vhost(TAPState *s, const NetdevTapOptions *tap,
const char *vhostfdname, Error **errp);
@@ -393,42 +394,6 @@ static NetClientInfo net_tap_info = {
.get_vhost_net = tap_get_vhost_net,
};
-static TAPState *net_tap_fd_init(NetClientState *peer,
- const char *model,
- const char *name,
- int fd,
- int vnet_hdr)
-{
- NetClientState *nc;
- TAPState *s;
-
- nc = qemu_new_net_client(&net_tap_info, peer, model, name);
-
- s = DO_UPCAST(TAPState, nc, nc);
-
- s->fd = fd;
- s->host_vnet_hdr_len = vnet_hdr ? sizeof(struct virtio_net_hdr) : 0;
- s->using_vnet_hdr = false;
- s->has_ufo = tap_probe_has_ufo(s->fd);
- s->has_uso = tap_probe_has_uso(s->fd);
- s->enabled = true;
- tap_set_offload(&s->nc, 0, 0, 0, 0, 0, 0, 0);
- /*
- * Make sure host header length is set correctly in tap:
- * it might have been modified by another instance of qemu.
- */
- if (vnet_hdr) {
- tap_fd_set_vnet_hdr_len(s->fd, s->host_vnet_hdr_len);
- }
- tap_read_poll(s, true);
- s->vhost_net = NULL;
-
- s->exit.notify = tap_exit_notify;
- qemu_add_exit_notifier(&s->exit);
-
- return s;
-}
-
static void close_all_fds_after_fork(int excluded_fd)
{
const int skip_fd[] = {STDIN_FILENO, STDOUT_FILENO, STDERR_FILENO,
@@ -651,9 +616,9 @@ static int net_tap_fd_init_external(const Netdev *netdev, NetClientState *peer,
}
}
- return net_init_tap_one(netdev, peer, model, name,
- NULL, NULL, NULL,
- vhostfdname, vnet_hdr, fd, errp);
+ return net_tap_fd_init_common(netdev, peer, model, name,
+ NULL, NULL, NULL,
+ vhostfdname, vnet_hdr, fd, errp);
}
int net_init_bridge(const Netdev *netdev, const char *name,
@@ -723,9 +688,9 @@ static int net_tap_open_one(const Netdev *netdev,
}
}
- ret = net_init_tap_one(netdev, peer, "tap", name, ifname,
- script, downscript,
- tap->vhostfd, vnet_hdr, fd, errp);
+ ret = net_tap_fd_init_common(netdev, peer, "tap", name, ifname,
+ script, downscript,
+ tap->vhostfd, vnet_hdr, fd, errp);
if (ret < 0) {
close(fd);
return -1;
@@ -736,15 +701,41 @@ static int net_tap_open_one(const Netdev *netdev,
#define MAX_TAP_QUEUES 1024
-static int net_init_tap_one(const Netdev *netdev, NetClientState *peer,
- const char *model, const char *name,
- const char *ifname, const char *script,
- const char *downscript, const char *vhostfdname,
- int vnet_hdr, int fd, Error **errp)
+static int net_tap_fd_init_common(const Netdev *netdev, NetClientState *peer,
+ const char *model, const char *name,
+ const char *ifname, const char *script,
+ const char *downscript,
+ const char *vhostfdname,
+ int vnet_hdr, int fd, Error **errp)
{
const NetdevTapOptions *tap;
- TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
int ret;
+ NetClientState *nc;
+ TAPState *s;
+
+ nc = qemu_new_net_client(&net_tap_info, peer, model, name);
+
+ s = DO_UPCAST(TAPState, nc, nc);
+
+ s->fd = fd;
+ s->host_vnet_hdr_len = vnet_hdr ? sizeof(struct virtio_net_hdr) : 0;
+ s->using_vnet_hdr = false;
+ s->has_ufo = tap_probe_has_ufo(s->fd);
+ s->has_uso = tap_probe_has_uso(s->fd);
+ s->enabled = true;
+ tap_set_offload(&s->nc, 0, 0, 0, 0, 0, 0, 0);
+ /*
+ * Make sure host header length is set correctly in tap:
+ * it might have been modified by another instance of qemu.
+ */
+ if (vnet_hdr) {
+ tap_fd_set_vnet_hdr_len(s->fd, s->host_vnet_hdr_len);
+ }
+ tap_read_poll(s, true);
+ s->vhost_net = NULL;
+
+ s->exit.notify = tap_exit_notify;
+ qemu_add_exit_notifier(&s->exit);
if (netdev->type == NET_CLIENT_DRIVER_BRIDGE) {
const NetdevBridgeOptions *bridge = &netdev->u.bridge;
--
2.48.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2 20/20] net/tap: introduce net_init_tap_fds()
2025-08-23 16:03 [PATCH v2 00/20] TAP initialization refactoring Vladimir Sementsov-Ogievskiy
` (18 preceding siblings ...)
2025-08-23 16:03 ` [PATCH v2 19/20] net/tap: drop net_tap_fd_init() Vladimir Sementsov-Ogievskiy
@ 2025-08-23 16:03 ` Vladimir Sementsov-Ogievskiy
2025-08-25 3:26 ` [PATCH v2 00/20] TAP initialization refactoring Lei Yang
2025-09-01 14:28 ` Vladimir Sementsov-Ogievskiy
21 siblings, 0 replies; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-08-23 16:03 UTC (permalink / raw)
To: jasowang; +Cc: qemu-devel, vsementsov, leiyang, steven.sistare
Final refactoring step for net_init_tap: move fds case to separate
function, so that net_init_tap() becomes straightforward top-level
entry point to tap initialization.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
net/tap.c | 68 +++++++++++++++++++++++++++++++------------------------
1 file changed, 39 insertions(+), 29 deletions(-)
diff --git a/net/tap.c b/net/tap.c
index 08927088eb..45d1bcd326 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -889,12 +889,48 @@ static int net_tap_open(const Netdev *netdev,
return 0;
}
+static int net_init_tap_fds(const Netdev *netdev, const char *name,
+ NetClientState *peer, Error **errp)
+{
+ const NetdevTapOptions *tap = &netdev->u.tap;
+ g_auto(GStrv) fds = NULL;
+ g_auto(GStrv) vhost_fds = NULL;
+ int nfds;
+ int vnet_hdr = 0, i = 0;
+ int ret;
+
+ assert(netdev->type == NET_CLIENT_DRIVER_TAP);
+
+ fds = g_strsplit(tap->fds, ":", MAX_TAP_QUEUES);
+ nfds = g_strv_length(fds);
+
+ if (tap->vhostfds) {
+ vhost_fds = g_strsplit(tap->vhostfds, ":", MAX_TAP_QUEUES);
+ if (nfds != g_strv_length(vhost_fds)) {
+ error_setg(errp, "The number of fds passed does not match "
+ "the number of vhostfds passed");
+ return -1;
+ }
+ }
+
+ vnet_hdr = -1;
+ for (i = 0; i < nfds; i++) {
+ ret = net_tap_from_monitor_fd(netdev, peer, name,
+ vhost_fds ? vhost_fds[i] : NULL,
+ &vnet_hdr, fds[i], errp);
+ if (ret < 0) {
+ return -1;
+ }
+ }
+
+ return 0;
+}
+
+
int net_init_tap(const Netdev *netdev, const char *name,
NetClientState *peer, Error **errp)
{
const NetdevTapOptions *tap = &netdev->u.tap;
- int vnet_hdr = 0, i = 0;
- int ret = 0;
assert(netdev->type == NET_CLIENT_DRIVER_TAP);
@@ -928,38 +964,12 @@ int net_init_tap(const Netdev *netdev, const char *name,
return net_tap_from_monitor_fd(netdev, peer, name, tap->vhostfd,
NULL, tap->fd, errp);
} else if (tap->fds) {
- g_auto(GStrv) fds = NULL;
- g_auto(GStrv) vhost_fds = NULL;
- int nfds;
-
if (tap->helper || tap->vhostfd) {
error_setg(errp, "helper= and vhostfd= are invalid with fds=");
return -1;
}
- fds = g_strsplit(tap->fds, ":", MAX_TAP_QUEUES);
- nfds = g_strv_length(fds);
-
- if (tap->vhostfds) {
- vhost_fds = g_strsplit(tap->vhostfds, ":", MAX_TAP_QUEUES);
- if (nfds != g_strv_length(vhost_fds)) {
- error_setg(errp, "The number of fds passed does not match "
- "the number of vhostfds passed");
- return -1;
- }
- }
-
- vnet_hdr = -1;
- for (i = 0; i < nfds; i++) {
- ret = net_tap_from_monitor_fd(netdev, peer, name,
- vhost_fds ? vhost_fds[i] : NULL,
- &vnet_hdr, fds[i], errp);
- if (ret < 0) {
- return -1;
- }
- }
-
- return 0;
+ return net_init_tap_fds(netdev, name, peer, errp);
} else if (tap->helper) {
if (tap->vhostfds) {
error_setg(errp, "vhostfds= is invalid with helper=");
--
2.48.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v2 00/20] TAP initialization refactoring
2025-08-23 16:03 [PATCH v2 00/20] TAP initialization refactoring Vladimir Sementsov-Ogievskiy
` (19 preceding siblings ...)
2025-08-23 16:03 ` [PATCH v2 20/20] net/tap: introduce net_init_tap_fds() Vladimir Sementsov-Ogievskiy
@ 2025-08-25 3:26 ` Lei Yang
2025-08-25 7:48 ` Vladimir Sementsov-Ogievskiy
2025-09-01 14:28 ` Vladimir Sementsov-Ogievskiy
21 siblings, 1 reply; 38+ messages in thread
From: Lei Yang @ 2025-08-25 3:26 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: jasowang, qemu-devel, steven.sistare
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
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 00/20] TAP initialization refactoring
2025-08-25 3:26 ` [PATCH v2 00/20] TAP initialization refactoring Lei Yang
@ 2025-08-25 7:48 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-08-25 7:48 UTC (permalink / raw)
To: Lei Yang; +Cc: jasowang, qemu-devel, steven.sistare
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 00/20] TAP initialization refactoring
2025-08-23 16:03 [PATCH v2 00/20] TAP initialization refactoring Vladimir Sementsov-Ogievskiy
` (20 preceding siblings ...)
2025-08-25 3:26 ` [PATCH v2 00/20] TAP initialization refactoring Lei Yang
@ 2025-09-01 14:28 ` Vladimir Sementsov-Ogievskiy
2025-09-02 9:09 ` Jason Wang
21 siblings, 1 reply; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-09-01 14:28 UTC (permalink / raw)
To: jasowang; +Cc: qemu-devel, leiyang, steven.sistare
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 00/20] TAP initialization refactoring
2025-09-01 14:28 ` Vladimir Sementsov-Ogievskiy
@ 2025-09-02 9:09 ` Jason Wang
0 siblings, 0 replies; 38+ messages in thread
From: Jason Wang @ 2025-09-02 9:09 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: qemu-devel, leiyang, steven.sistare
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
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 02/20] net/tap: add set_fd_nonblocking() helper
2025-08-23 16:03 ` [PATCH v2 02/20] net/tap: add set_fd_nonblocking() helper Vladimir Sementsov-Ogievskiy
@ 2025-09-02 12:32 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-09-02 12:32 UTC (permalink / raw)
To: jasowang; +Cc: qemu-devel, leiyang, steven.sistare
On 23.08.25 19:03, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Skip this one, I'm preparing a separate series to improve set_blocking/set_nonblocking code everywhere.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 01/20] net/tap: net_init_tap_one(): add return value
2025-08-23 16:03 ` [PATCH v2 01/20] net/tap: net_init_tap_one(): add return value Vladimir Sementsov-Ogievskiy
@ 2025-09-03 4:04 ` Jason Wang
2025-09-03 6:58 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 38+ messages in thread
From: Jason Wang @ 2025-09-03 4:04 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: qemu-devel, leiyang, steven.sistare
On Sun, Aug 24, 2025 at 12:03 AM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> To avoid error propagation, let's follow common recommendation to
> use return value together with errp.
It would be better to have some words or links to explain why we need
to avoid error propagation.
>
> Probably, it would also be good to use bool as a return type
> (switching to true/false as success/failure instead of 0/-1). But
> seems almost all functions (including a lot of them with errp
> argument) have 0/-1 semantics in net/, so making exclusions doesn't
> seem good. If we want such a switch, we should update the whole
> net/ directory.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
Thanks
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 05/20] net/tap: net_init_tap_one(): move parameter checking earlier
2025-08-23 16:03 ` [PATCH v2 05/20] net/tap: net_init_tap_one(): move parameter checking earlier Vladimir Sementsov-Ogievskiy
@ 2025-09-03 4:18 ` Jason Wang
2025-09-03 7:02 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 38+ messages in thread
From: Jason Wang @ 2025-09-03 4:18 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: qemu-devel, leiyang, steven.sistare
On Sun, Aug 24, 2025 at 12:03 AM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> Let's keep all similar argument checking in net_init_tap() function.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> net/tap.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/net/tap.c b/net/tap.c
> index 58c3318b1c..3fe99ef63f 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -765,9 +765,6 @@ static int net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
> "vhost-net requested but could not be initialized");
> goto failed;
> }
> - } else if (vhostfdname) {
> - error_setg(errp, "vhostfd(s)= is not valid without vhost");
> - goto failed;
> }
>
> return 0;
> @@ -829,6 +826,11 @@ int net_init_tap(const Netdev *netdev, const char *name,
> return -1;
> }
>
> + if (tap->has_vhost && !tap->vhost && (tap->vhostfds || tap->vhostfd)) {
> + error_setg(errp, "vhostfd(s)= is not valid without vhost");
> + return -1;
> + }
So this doesn't deal with vhost force or is this expected to be
applied on top of the deprecation of vhostforce?
Thanks
> +
> if (tap->fd) {
> if (tap->ifname || tap->script || tap->downscript ||
> tap->has_vnet_hdr || tap->helper || tap->has_queues ||
> --
> 2.48.1
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 11/20] net/tap: net_init_tap_one(): refactor to use netdev as first arg
2025-08-23 16:03 ` [PATCH v2 11/20] net/tap: net_init_tap_one(): refactor to use netdev as first arg Vladimir Sementsov-Ogievskiy
@ 2025-09-03 5:19 ` Jason Wang
2025-09-03 7:23 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 38+ messages in thread
From: Jason Wang @ 2025-09-03 5:19 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: qemu-devel, leiyang, steven.sistare
On Sun, Aug 24, 2025 at 12:03 AM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> This is needed for further unification of bridge initialization in
> net_init_tap() and net_init_bridge().
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> net/tap.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/net/tap.c b/net/tap.c
> index eab0a86173..468dae7004 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -692,15 +692,18 @@ static int net_tap_init(const NetdevTapOptions *tap, int *vnet_hdr,
>
> #define MAX_TAP_QUEUES 1024
>
> -static int net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
> +static int net_init_tap_one(const Netdev *netdev, NetClientState *peer,
> const char *model, const char *name,
> const char *ifname, const char *script,
> const char *downscript, const char *vhostfdname,
> int vnet_hdr, int fd, Error **errp)
> {
> + const NetdevTapOptions *tap = &netdev->u.tap;
> TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
> int vhostfd;
>
> + assert(netdev->type == NET_CLIENT_DRIVER_TAP);
> +
I think we should try our best to avoid assertions and return errors here.
Thanks
> if (tap_set_sndbuf(s->fd, tap, errp) < 0) {
> goto failed;
> }
> @@ -826,7 +829,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
> return -1;
> }
>
> - ret = net_init_tap_one(tap, peer, "tap", name, NULL,
> + ret = net_init_tap_one(netdev, peer, "tap", name, NULL,
> NULL, NULL,
> tap->vhostfd, vnet_hdr, fd, errp);
> if (ret < 0) {
> @@ -875,7 +878,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
> return -1;
> }
>
> - ret = net_init_tap_one(tap, peer, "tap", name, NULL,
> + ret = net_init_tap_one(netdev, peer, "tap", name, NULL,
> NULL, NULL,
> vhost_fds ? vhost_fds[i] : NULL,
> vnet_hdr, fd, errp);
> @@ -905,7 +908,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
> return -1;
> }
>
> - ret = net_init_tap_one(tap, peer, "bridge", name, NULL,
> + ret = net_init_tap_one(netdev, peer, "bridge", name, NULL,
> NULL, NULL, tap->vhostfd,
> vnet_hdr, fd, errp);
> if (ret < 0) {
> @@ -946,7 +949,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
> return -1;
> }
>
> - ret = net_init_tap_one(tap, peer, "tap", name, ifname,
> + ret = net_init_tap_one(netdev, peer, "tap", name, ifname,
> i >= 1 ? "no" : script,
> i >= 1 ? "no" : downscript,
> tap->vhostfd, vnet_hdr, fd, errp);
> --
> 2.48.1
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 12/20] net/tap: net_init_tap_one(): support bridge
2025-08-23 16:03 ` [PATCH v2 12/20] net/tap: net_init_tap_one(): support bridge Vladimir Sementsov-Ogievskiy
@ 2025-09-03 5:28 ` Jason Wang
2025-09-03 7:27 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 38+ messages in thread
From: Jason Wang @ 2025-09-03 5:28 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: qemu-devel, leiyang, steven.sistare
On Sun, Aug 24, 2025 at 12:03 AM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> Use net_init_tap_one() in net_init_bridge().
Need to explain why this is needed.
Patch looks good.
Thanks
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> net/tap.c | 28 +++++++++++++++++++++++-----
> 1 file changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/net/tap.c b/net/tap.c
> index 468dae7004..5acfb128a1 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -88,6 +88,12 @@ static void launch_script(const char *setup_script, const char *ifname,
> static void tap_send(void *opaque);
> static void tap_writable(void *opaque);
>
> +static int net_init_tap_one(const Netdev *netdev, NetClientState *peer,
> + const char *model, const char *name,
> + const char *ifname, const char *script,
> + const char *downscript, const char *vhostfdname,
> + int vnet_hdr, int fd, Error **errp);
> +
> static void tap_update_fd_handler(TAPState *s)
> {
> qemu_set_fd_handler(s->fd,
> @@ -626,8 +632,7 @@ int net_init_bridge(const Netdev *netdev, const char *name,
> {
> const NetdevBridgeOptions *bridge;
> const char *helper, *br;
> - TAPState *s;
> - int fd, vnet_hdr;
> + int fd, vnet_hdr, ret;
>
> assert(netdev->type == NET_CLIENT_DRIVER_BRIDGE);
> bridge = &netdev->u.bridge;
> @@ -648,9 +653,14 @@ int net_init_bridge(const Netdev *netdev, const char *name,
> close(fd);
> return -1;
> }
> - s = net_tap_fd_init(peer, "bridge", name, fd, vnet_hdr);
>
> - qemu_set_info_str(&s->nc, "helper=%s,br=%s", helper, br);
> + ret = net_init_tap_one(netdev, peer, "bridge", name,
> + NULL, NULL, NULL,
> + NULL, vnet_hdr, fd, errp);
> + if (ret < 0) {
> + close(fd);
> + return -1;
> + }
>
> return 0;
> }
> @@ -698,11 +708,19 @@ static int net_init_tap_one(const Netdev *netdev, NetClientState *peer,
> const char *downscript, const char *vhostfdname,
> int vnet_hdr, int fd, Error **errp)
> {
> - const NetdevTapOptions *tap = &netdev->u.tap;
> + const NetdevTapOptions *tap;
> TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
> int vhostfd;
>
> + if (netdev->type == NET_CLIENT_DRIVER_BRIDGE) {
> + const NetdevBridgeOptions *bridge = &netdev->u.bridge;
> + qemu_set_info_str(&s->nc, "helper=%s,br=%s",
> + bridge->helper, bridge->br);
> + return 0;
> + }
> +
> assert(netdev->type == NET_CLIENT_DRIVER_TAP);
> + tap = &netdev->u.tap;
>
> if (tap_set_sndbuf(s->fd, tap, errp) < 0) {
> goto failed;
> --
> 2.48.1
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 14/20] net/tap: refactor net_tap_init() into net_tap_open_one()
2025-08-23 16:03 ` [PATCH v2 14/20] net/tap: refactor net_tap_init() into net_tap_open_one() Vladimir Sementsov-Ogievskiy
@ 2025-09-03 5:36 ` Jason Wang
2025-09-03 7:33 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 38+ messages in thread
From: Jason Wang @ 2025-09-03 5:36 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: qemu-devel, leiyang, steven.sistare
On Sun, Aug 24, 2025 at 12:03 AM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> net_tap_init() is used in one place. Let's move net_init_tap_one()
> call to it and simplify outer loop code.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> net/tap.c | 54 +++++++++++++++++++++++++++++++-----------------------
> 1 file changed, 31 insertions(+), 23 deletions(-)
>
> diff --git a/net/tap.c b/net/tap.c
> index 83a1c9250a..57939ed16f 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -674,31 +674,37 @@ int net_init_bridge(const Netdev *netdev, const char *name,
> return 0;
> }
>
> -static int net_tap_init(const NetdevTapOptions *tap, int *vnet_hdr,
> - const char *setup_script, char *ifname,
> - size_t ifname_sz, int mq_required, Error **errp)
> +static int net_tap_open_one(const Netdev *netdev,
> + const char *name, NetClientState *peer,
> + const char *script, const char *downscript,
I'd stick to "setup_script" as we have "downscript".
And we can save several lines of changes.
The rest looks good.
Thanks
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 16/20] net/tap: introduce net_tap_fd_init_external()
2025-08-23 16:03 ` [PATCH v2 16/20] net/tap: introduce net_tap_fd_init_external() Vladimir Sementsov-Ogievskiy
@ 2025-09-03 5:37 ` Jason Wang
2025-09-03 7:45 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 38+ messages in thread
From: Jason Wang @ 2025-09-03 5:37 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: qemu-devel, leiyang, steven.sistare
On Sun, Aug 24, 2025 at 12:03 AM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> Add helper that covers logic for initializing fds, given from monitor
> or helper.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> net/tap.c | 90 ++++++++++++++++++++++---------------------------------
> 1 file changed, 36 insertions(+), 54 deletions(-)
>
> diff --git a/net/tap.c b/net/tap.c
> index 27642c45a9..8cea6ed87b 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -627,13 +627,40 @@ static bool set_fd_nonblocking(int fd, const char *note, Error **errp)
> return ok;
> }
>
> +static int net_tap_fd_init_external(const Netdev *netdev, NetClientState *peer,
> + const char *model, const char *name,
> + const char *vhostfdname,
> + int *pvnet_hdr, int fd, Error **errp)
Is net_tap_fd_init_mon() better?
Thanks
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 01/20] net/tap: net_init_tap_one(): add return value
2025-09-03 4:04 ` Jason Wang
@ 2025-09-03 6:58 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-09-03 6:58 UTC (permalink / raw)
To: Jason Wang; +Cc: qemu-devel, leiyang, steven.sistare
On 03.09.25 07:04, Jason Wang wrote:
> On Sun, Aug 24, 2025 at 12:03 AM Vladimir Sementsov-Ogievskiy
> <vsementsov@yandex-team.ru> wrote:
>>
>> To avoid error propagation, let's follow common recommendation to
>> use return value together with errp.
>
> It would be better to have some words or links to explain why we need
> to avoid error propagation.
In short with additional return value we get:
- less code to handle error
- don't create and set Error object when it's not required (when passed errp=NULL)
More details in commit message of e3fe3988d7851cac3 "error: Document Error API usage rules".
I'll add this information here.
>
>>
>> Probably, it would also be good to use bool as a return type
>> (switching to true/false as success/failure instead of 0/-1). But
>> seems almost all functions (including a lot of them with errp
>> argument) have 0/-1 semantics in net/, so making exclusions doesn't
>> seem good. If we want such a switch, we should update the whole
>> net/ directory.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>
> Thanks
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 05/20] net/tap: net_init_tap_one(): move parameter checking earlier
2025-09-03 4:18 ` Jason Wang
@ 2025-09-03 7:02 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-09-03 7:02 UTC (permalink / raw)
To: Jason Wang; +Cc: qemu-devel, leiyang, steven.sistare
On 03.09.25 07:18, Jason Wang wrote:
> On Sun, Aug 24, 2025 at 12:03 AM Vladimir Sementsov-Ogievskiy
> <vsementsov@yandex-team.ru> wrote:
>>
>> Let's keep all similar argument checking in net_init_tap() function.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>> net/tap.c | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/tap.c b/net/tap.c
>> index 58c3318b1c..3fe99ef63f 100644
>> --- a/net/tap.c
>> +++ b/net/tap.c
>> @@ -765,9 +765,6 @@ static int net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
>> "vhost-net requested but could not be initialized");
>> goto failed;
>> }
>> - } else if (vhostfdname) {
>> - error_setg(errp, "vhostfd(s)= is not valid without vhost");
>> - goto failed;
>> }
>>
>> return 0;
>> @@ -829,6 +826,11 @@ int net_init_tap(const Netdev *netdev, const char *name,
>> return -1;
>> }
>>
>> + if (tap->has_vhost && !tap->vhost && (tap->vhostfds || tap->vhostfd)) {
>> + error_setg(errp, "vhostfd(s)= is not valid without vhost");
>> + return -1;
>> + }
>
> So this doesn't deal with vhost force or is this expected to be
> applied on top of the deprecation of vhostforce?
>
This patch doesn't modify existing logic. And deprecation doesn't modify logic in tap.c as well.
So, this series and deprecation may be applied in any order.
>
>> +
>> if (tap->fd) {
>> if (tap->ifname || tap->script || tap->downscript ||
>> tap->has_vnet_hdr || tap->helper || tap->has_queues ||
>> --
>> 2.48.1
>>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 11/20] net/tap: net_init_tap_one(): refactor to use netdev as first arg
2025-09-03 5:19 ` Jason Wang
@ 2025-09-03 7:23 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-09-03 7:23 UTC (permalink / raw)
To: Jason Wang; +Cc: qemu-devel, leiyang, steven.sistare
On 03.09.25 08:19, Jason Wang wrote:
> On Sun, Aug 24, 2025 at 12:03 AM Vladimir Sementsov-Ogievskiy
> <vsementsov@yandex-team.ru> wrote:
>>
>> This is needed for further unification of bridge initialization in
>> net_init_tap() and net_init_bridge().
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>> net/tap.c | 13 ++++++++-----
>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/tap.c b/net/tap.c
>> index eab0a86173..468dae7004 100644
>> --- a/net/tap.c
>> +++ b/net/tap.c
>> @@ -692,15 +692,18 @@ static int net_tap_init(const NetdevTapOptions *tap, int *vnet_hdr,
>>
>> #define MAX_TAP_QUEUES 1024
>>
>> -static int net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
>> +static int net_init_tap_one(const Netdev *netdev, NetClientState *peer,
>> const char *model, const char *name,
>> const char *ifname, const char *script,
>> const char *downscript, const char *vhostfdname,
>> int vnet_hdr, int fd, Error **errp)
>> {
>> + const NetdevTapOptions *tap = &netdev->u.tap;
>> TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
>> int vhostfd;
>>
>> + assert(netdev->type == NET_CLIENT_DRIVER_TAP);
>> +
>
> I think we should try our best to avoid assertions and return errors here.
>
Hmm. But why? This is a static function that we call only for TAP. The error here
is not logically possible.
The error here would mean:
- either we have a terrible bug in code, so we don't understand which functions call which ones, so better to stop here (abort)
- or it's a kind of memory corruption, and again, better to abort than continue damage users data
The assert may be removed not breaking the logic. But it brings the best kind of documentation,
that in this function we work only with TAP.
Finally, there a lot of similar asserts already in net code and in tap.c:
git grep 'assert.*== NET_CLIENT' | wc -l
45
>
>> if (tap_set_sndbuf(s->fd, tap, errp) < 0) {
>> goto failed;
>> }
>> @@ -826,7 +829,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
>> return -1;
>> }
>>
>> - ret = net_init_tap_one(tap, peer, "tap", name, NULL,
>> + ret = net_init_tap_one(netdev, peer, "tap", name, NULL,
>> NULL, NULL,
>> tap->vhostfd, vnet_hdr, fd, errp);
>> if (ret < 0) {
>> @@ -875,7 +878,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
>> return -1;
>> }
>>
>> - ret = net_init_tap_one(tap, peer, "tap", name, NULL,
>> + ret = net_init_tap_one(netdev, peer, "tap", name, NULL,
>> NULL, NULL,
>> vhost_fds ? vhost_fds[i] : NULL,
>> vnet_hdr, fd, errp);
>> @@ -905,7 +908,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
>> return -1;
>> }
>>
>> - ret = net_init_tap_one(tap, peer, "bridge", name, NULL,
>> + ret = net_init_tap_one(netdev, peer, "bridge", name, NULL,
>> NULL, NULL, tap->vhostfd,
>> vnet_hdr, fd, errp);
>> if (ret < 0) {
>> @@ -946,7 +949,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
>> return -1;
>> }
>>
>> - ret = net_init_tap_one(tap, peer, "tap", name, ifname,
>> + ret = net_init_tap_one(netdev, peer, "tap", name, ifname,
>> i >= 1 ? "no" : script,
>> i >= 1 ? "no" : downscript,
>> tap->vhostfd, vnet_hdr, fd, errp);
>> --
>> 2.48.1
>>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 12/20] net/tap: net_init_tap_one(): support bridge
2025-09-03 5:28 ` Jason Wang
@ 2025-09-03 7:27 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-09-03 7:27 UTC (permalink / raw)
To: Jason Wang; +Cc: qemu-devel, leiyang, steven.sistare
On 03.09.25 08:28, Jason Wang wrote:
> On Sun, Aug 24, 2025 at 12:03 AM Vladimir Sementsov-Ogievskiy
> <vsementsov@yandex-team.ru> wrote:
>>
>> Use net_init_tap_one() in net_init_bridge().
>
> Need to explain why this is needed.
>
Oh, right. It took a minute for me to remember)
Will add:
To be able in further commits use net_init_bridge for "bridge" case of init_tap,
as it almost the same, but requires tap-related things in net_init_tap_one.
>
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>> net/tap.c | 28 +++++++++++++++++++++++-----
>> 1 file changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/tap.c b/net/tap.c
>> index 468dae7004..5acfb128a1 100644
>> --- a/net/tap.c
>> +++ b/net/tap.c
>> @@ -88,6 +88,12 @@ static void launch_script(const char *setup_script, const char *ifname,
>> static void tap_send(void *opaque);
>> static void tap_writable(void *opaque);
>>
>> +static int net_init_tap_one(const Netdev *netdev, NetClientState *peer,
>> + const char *model, const char *name,
>> + const char *ifname, const char *script,
>> + const char *downscript, const char *vhostfdname,
>> + int vnet_hdr, int fd, Error **errp);
>> +
>> static void tap_update_fd_handler(TAPState *s)
>> {
>> qemu_set_fd_handler(s->fd,
>> @@ -626,8 +632,7 @@ int net_init_bridge(const Netdev *netdev, const char *name,
>> {
>> const NetdevBridgeOptions *bridge;
>> const char *helper, *br;
>> - TAPState *s;
>> - int fd, vnet_hdr;
>> + int fd, vnet_hdr, ret;
>>
>> assert(netdev->type == NET_CLIENT_DRIVER_BRIDGE);
>> bridge = &netdev->u.bridge;
>> @@ -648,9 +653,14 @@ int net_init_bridge(const Netdev *netdev, const char *name,
>> close(fd);
>> return -1;
>> }
>> - s = net_tap_fd_init(peer, "bridge", name, fd, vnet_hdr);
>>
>> - qemu_set_info_str(&s->nc, "helper=%s,br=%s", helper, br);
>> + ret = net_init_tap_one(netdev, peer, "bridge", name,
>> + NULL, NULL, NULL,
>> + NULL, vnet_hdr, fd, errp);
>> + if (ret < 0) {
>> + close(fd);
>> + return -1;
>> + }
>>
>> return 0;
>> }
>> @@ -698,11 +708,19 @@ static int net_init_tap_one(const Netdev *netdev, NetClientState *peer,
>> const char *downscript, const char *vhostfdname,
>> int vnet_hdr, int fd, Error **errp)
>> {
>> - const NetdevTapOptions *tap = &netdev->u.tap;
>> + const NetdevTapOptions *tap;
>> TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
>> int vhostfd;
>>
>> + if (netdev->type == NET_CLIENT_DRIVER_BRIDGE) {
>> + const NetdevBridgeOptions *bridge = &netdev->u.bridge;
>> + qemu_set_info_str(&s->nc, "helper=%s,br=%s",
>> + bridge->helper, bridge->br);
>> + return 0;
>> + }
>> +
>> assert(netdev->type == NET_CLIENT_DRIVER_TAP);
>> + tap = &netdev->u.tap;
>>
>> if (tap_set_sndbuf(s->fd, tap, errp) < 0) {
>> goto failed;
>> --
>> 2.48.1
>>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 14/20] net/tap: refactor net_tap_init() into net_tap_open_one()
2025-09-03 5:36 ` Jason Wang
@ 2025-09-03 7:33 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-09-03 7:33 UTC (permalink / raw)
To: Jason Wang; +Cc: qemu-devel, leiyang, steven.sistare
On 03.09.25 08:36, Jason Wang wrote:
> On Sun, Aug 24, 2025 at 12:03 AM Vladimir Sementsov-Ogievskiy
> <vsementsov@yandex-team.ru> wrote:
>>
>> net_tap_init() is used in one place. Let's move net_init_tap_one()
>> call to it and simplify outer loop code.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>> net/tap.c | 54 +++++++++++++++++++++++++++++++-----------------------
>> 1 file changed, 31 insertions(+), 23 deletions(-)
>>
>> diff --git a/net/tap.c b/net/tap.c
>> index 83a1c9250a..57939ed16f 100644
>> --- a/net/tap.c
>> +++ b/net/tap.c
>> @@ -674,31 +674,37 @@ int net_init_bridge(const Netdev *netdev, const char *name,
>> return 0;
>> }
>>
>> -static int net_tap_init(const NetdevTapOptions *tap, int *vnet_hdr,
>> - const char *setup_script, char *ifname,
>> - size_t ifname_sz, int mq_required, Error **errp)
>> +static int net_tap_open_one(const Netdev *netdev,
>> + const char *name, NetClientState *peer,
>> + const char *script, const char *downscript,
>
> I'd stick to "setup_script" as we have "downscript".
I decided to rename, because in other places the variable is called script, and the original option is also script=.
It would be more correct to rename in a separate commit. I can do this in v2, or just add a not to commit message here.
>
> And we can save several lines of changes.
>
> The rest looks good.
>
> Thanks
>
Thank you for reviewing!
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 16/20] net/tap: introduce net_tap_fd_init_external()
2025-09-03 5:37 ` Jason Wang
@ 2025-09-03 7:45 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 38+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-09-03 7:45 UTC (permalink / raw)
To: Jason Wang; +Cc: qemu-devel, leiyang, steven.sistare
On 03.09.25 08:37, Jason Wang wrote:
> On Sun, Aug 24, 2025 at 12:03 AM Vladimir Sementsov-Ogievskiy
> <vsementsov@yandex-team.ru> wrote:
>>
>> Add helper that covers logic for initializing fds, given from monitor
>> or helper.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>> net/tap.c | 90 ++++++++++++++++++++++---------------------------------
>> 1 file changed, 36 insertions(+), 54 deletions(-)
>>
>> diff --git a/net/tap.c b/net/tap.c
>> index 27642c45a9..8cea6ed87b 100644
>> --- a/net/tap.c
>> +++ b/net/tap.c
>> @@ -627,13 +627,40 @@ static bool set_fd_nonblocking(int fd, const char *note, Error **errp)
>> return ok;
>> }
>>
>> +static int net_tap_fd_init_external(const Netdev *netdev, NetClientState *peer,
>> + const char *model, const char *name,
>> + const char *vhostfdname,
>> + int *pvnet_hdr, int fd, Error **errp)
>
> Is net_tap_fd_init_mon() better?
>
The function is shared between "monitor" case and "helper" case.
I just didn't look what "helper" actually do, and decided that fd comes from somewhere...
Now looking at code, I see that for helper we do fork(), and exec() the helper in child process, and then read
the fd in parent from preliminary created socket pair.
So I still think that "external" works good: either we get fd from monitor, or from third "helper" program.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 38+ messages in thread
end of thread, other threads:[~2025-09-03 7:45 UTC | newest]
Thread overview: 38+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-23 16:03 [PATCH v2 00/20] TAP initialization refactoring Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 01/20] net/tap: net_init_tap_one(): add return value Vladimir Sementsov-Ogievskiy
2025-09-03 4:04 ` Jason Wang
2025-09-03 6:58 ` Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 02/20] net/tap: add set_fd_nonblocking() helper Vladimir Sementsov-Ogievskiy
2025-09-02 12:32 ` Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 03/20] net/tap: tap_set_sndbuf(): add return value Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 04/20] net/tap: net_init_tap_one(): drop extra error propagation Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 05/20] net/tap: net_init_tap_one(): move parameter checking earlier Vladimir Sementsov-Ogievskiy
2025-09-03 4:18 ` Jason Wang
2025-09-03 7:02 ` Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 06/20] net/tap: net_init_tap(): refactor parameter checking Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 07/20] net/tap: net_init_tap(): drop extra variable vhostfdname Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 08/20] net/tap: move local variables related to the latter case to else branch Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 09/20] net/tap: use glib strings vector and g_strsplit for fds case Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 10/20] net/tap: drop extra tap_fd_get_ifname() call Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 11/20] net/tap: net_init_tap_one(): refactor to use netdev as first arg Vladimir Sementsov-Ogievskiy
2025-09-03 5:19 ` Jason Wang
2025-09-03 7:23 ` Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 12/20] net/tap: net_init_tap_one(): support bridge Vladimir Sementsov-Ogievskiy
2025-09-03 5:28 ` Jason Wang
2025-09-03 7:27 ` Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 13/20] net/tap: net_init_bridge(): support tap Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 14/20] net/tap: refactor net_tap_init() into net_tap_open_one() Vladimir Sementsov-Ogievskiy
2025-09-03 5:36 ` Jason Wang
2025-09-03 7:33 ` Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 15/20] net/tap: introduce net_tap_open() Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 16/20] net/tap: introduce net_tap_fd_init_external() Vladimir Sementsov-Ogievskiy
2025-09-03 5:37 ` Jason Wang
2025-09-03 7:45 ` Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 17/20] net/tap: introduce net_tap_from_monitor_fd() helper Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 18/20] net/tap: split net_tap_setup_vhost() separate function Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 19/20] net/tap: drop net_tap_fd_init() Vladimir Sementsov-Ogievskiy
2025-08-23 16:03 ` [PATCH v2 20/20] net/tap: introduce net_init_tap_fds() Vladimir Sementsov-Ogievskiy
2025-08-25 3:26 ` [PATCH v2 00/20] TAP initialization refactoring Lei Yang
2025-08-25 7:48 ` Vladimir Sementsov-Ogievskiy
2025-09-01 14:28 ` Vladimir Sementsov-Ogievskiy
2025-09-02 9:09 ` Jason Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).