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