| Message ID | 20200512123149.30207-3-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: Drop legacy "name" from -net and remove NetLegacy | expand |
On 5/12/20 7:31 AM, Thomas Huth wrote: > Now that the "name" parameter is gone, there is hardly any difference > between NetLegacy and Netdev anymore. Drop NetLegacy and always use > Netdev to simplify the code quite a bit. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > +++ b/net/net.c > @@ -967,13 +967,14 @@ static int (* const net_client_init_fun[NET_CLIENT_DRIVER__MAX])( > > static int net_client_init1(const void *object, bool is_netdev, Error **errp) Why do we still need the 'is_netdev' parameter? If all callers are passing in a netdev, then either this parameter needs to be renamed to capture the actual difference between callers, or it can be dropped altogether. > { > - Netdev legacy = {0}; > - const Netdev *netdev; > + const Netdev *netdev = object; > NetClientState *peer = NULL; > > if (is_netdev) { > - netdev = object; > - > + if (!netdev->has_id) { > + error_setg(errp, QERR_MISSING_PARAMETER, "id"); > + return -1; > + } You wouldn't need this if 'id' remained mandatory. > @@ -981,56 +982,11 @@ static int net_client_init1(const void *object, bool is_netdev, Error **errp) > return -1; > } > } else { > - const NetLegacy *net = object; > - const NetLegacyOptions *opts = net->opts; > - legacy.id = net->id; > - netdev = &legacy; > - > - /* Map the old options to the new flat type */ > - switch (opts->type) { > - case NET_LEGACY_OPTIONS_TYPE_NONE: > + if (netdev->type == NET_CLIENT_DRIVER_NONE) { > return 0; /* nothing to do */ > - > - if (!net_client_init_fun[netdev->type]) { > + if (netdev->type == NET_CLIENT_DRIVER_HUBPORT || > + !net_client_init_fun[netdev->type]) { > error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type", > "a net backend type (maybe it is not compiled " > "into this binary)"); So maybe we still want this legacy-handling code, but renaming 'is_netdev' to 'legacy_handling' may make more sense. > @@ -1039,7 +995,7 @@ static int net_client_init1(const void *object, bool is_netdev, Error **errp) > > /* Do not add to a hub if it's a nic with a netdev= parameter. */ > if (netdev->type != NET_CLIENT_DRIVER_NIC || > - !opts->u.nic.has_netdev) { > + !netdev->u.nic.has_netdev) { > peer = net_hub_add_port(0, NULL, NULL); > } > } > @@ -1143,21 +1099,13 @@ static int net_client_init(QemuOpts *opts, bool is_netdev, Error **errp) > } > } > > - if (is_netdev) { > - visit_type_Netdev(v, NULL, (Netdev **)&object, &err); > - } else { > - visit_type_NetLegacy(v, NULL, (NetLegacy **)&object, &err); > - } > + visit_type_Netdev(v, NULL, (Netdev **)&object, &err); Why do we need to cast? If all callers are passing a Netdev, can't we just give 'object' the correct type to begin with? > +++ b/qapi/net.json > @@ -453,7 +453,7 @@ > # 'l2tpv3' - since 2.1 > ## > { 'union': 'Netdev', > - 'base': { 'id': 'str', 'type': 'NetClientDriver' }, > + 'base': { '*id': 'str', 'type': 'NetClientDriver' }, I don't think we need to make 'id' optional. > 'discriminator': 'type', > 'data': { > 'nic': 'NetLegacyNicOptions', > @@ -467,52 +467,6 @@ > 'netmap': 'NetdevNetmapOptions', > 'vhost-user': 'NetdevVhostUserOptions' } } > > -## > -# @NetLegacy: > -# > -# Captures the configuration of a network device; legacy. > -# > -# @id: identifier for monitor commands > -# > -# @opts: device type specific properties (legacy) > -# > -# Since: 1.2 > -## > -{ 'struct': 'NetLegacy', > - 'data': { > - '*id': 'str', > - 'opts': 'NetLegacyOptions' } } > - > -## > -# @NetLegacyOptionsType: > -# > -# Since: 1.2 > -## > -{ 'enum': 'NetLegacyOptionsType', > - 'data': ['none', 'nic', 'user', 'tap', 'l2tpv3', 'socket', 'vde', > - 'bridge', 'netmap', 'vhost-user'] } NetLegacyOptionsType differs from NetClientDriver only by missing 'hubport', which your code above special-cased, so on that front, the simplification makes sense.
On 12/05/2020 16.32, Eric Blake wrote: > On 5/12/20 7:31 AM, Thomas Huth wrote: >> Now that the "name" parameter is gone, there is hardly any difference >> between NetLegacy and Netdev anymore. Drop NetLegacy and always use >> Netdev to simplify the code quite a bit. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- > >> +++ b/net/net.c >> @@ -967,13 +967,14 @@ static int (* const >> net_client_init_fun[NET_CLIENT_DRIVER__MAX])( >> static int net_client_init1(const void *object, bool is_netdev, >> Error **errp) > > Why do we still need the 'is_netdev' parameter? Yes. See net_init_client(), it uses "false" here. > If all callers are > passing in a netdev, then either this parameter needs to be renamed to > capture the actual difference between callers, or it can be dropped > altogether. Don't think of the "Netdev" structure when you're looking at "is_netdev", rather think about the "-netdev" comand line parameter. "is_netdev" is used to distinguish between "-netdev" and "-net". >> { >> - Netdev legacy = {0}; >> - const Netdev *netdev; >> + const Netdev *netdev = object; >> NetClientState *peer = NULL; >> if (is_netdev) { >> - netdev = object; >> - >> + if (!netdev->has_id) { >> + error_setg(errp, QERR_MISSING_PARAMETER, "id"); >> + return -1; >> + } > > You wouldn't need this if 'id' remained mandatory. It's still required for "-net" - see my reply to patch 1/2. >> @@ -981,56 +982,11 @@ static int net_client_init1(const void *object, >> bool is_netdev, Error **errp) >> return -1; >> } >> } else { >> - const NetLegacy *net = object; >> - const NetLegacyOptions *opts = net->opts; >> - legacy.id = net->id; >> - netdev = &legacy; >> - >> - /* Map the old options to the new flat type */ >> - switch (opts->type) { >> - case NET_LEGACY_OPTIONS_TYPE_NONE: >> + if (netdev->type == NET_CLIENT_DRIVER_NONE) { >> return 0; /* nothing to do */ > > >> - >> - if (!net_client_init_fun[netdev->type]) { >> + if (netdev->type == NET_CLIENT_DRIVER_HUBPORT || >> + !net_client_init_fun[netdev->type]) { >> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type", >> "a net backend type (maybe it is not compiled " >> "into this binary)"); > > So maybe we still want this legacy-handling code, but renaming > 'is_netdev' to 'legacy_handling' may make more sense. Hmm, maybe renaming would be good indeed to avoid confusion here... I'll think about it. >> @@ -1039,7 +995,7 @@ static int net_client_init1(const void *object, >> bool is_netdev, Error **errp) >> /* Do not add to a hub if it's a nic with a netdev= >> parameter. */ >> if (netdev->type != NET_CLIENT_DRIVER_NIC || >> - !opts->u.nic.has_netdev) { >> + !netdev->u.nic.has_netdev) { >> peer = net_hub_add_port(0, NULL, NULL); >> } >> } >> @@ -1143,21 +1099,13 @@ static int net_client_init(QemuOpts *opts, >> bool is_netdev, Error **errp) >> } >> } >> - if (is_netdev) { >> - visit_type_Netdev(v, NULL, (Netdev **)&object, &err); >> - } else { >> - visit_type_NetLegacy(v, NULL, (NetLegacy **)&object, &err); >> - } >> + visit_type_Netdev(v, NULL, (Netdev **)&object, &err); > > Why do we need to cast? If all callers are passing a Netdev, can't we > just give 'object' the correct type to begin with? Agreed, it should be possible to use Netdev* instead of void* for "object" now. I'll change that in v3. >> +++ b/qapi/net.json >> @@ -453,7 +453,7 @@ >> # 'l2tpv3' - since 2.1 >> ## >> { 'union': 'Netdev', >> - 'base': { 'id': 'str', 'type': 'NetClientDriver' }, >> + 'base': { '*id': 'str', 'type': 'NetClientDriver' }, > > I don't think we need to make 'id' optional. It's required for "-net" now. >> 'discriminator': 'type', >> 'data': { >> 'nic': 'NetLegacyNicOptions', >> @@ -467,52 +467,6 @@ >> 'netmap': 'NetdevNetmapOptions', >> 'vhost-user': 'NetdevVhostUserOptions' } } >> -## >> -# @NetLegacy: >> -# >> -# Captures the configuration of a network device; legacy. >> -# >> -# @id: identifier for monitor commands >> -# >> -# @opts: device type specific properties (legacy) >> -# >> -# Since: 1.2 >> -## >> -{ 'struct': 'NetLegacy', >> - 'data': { >> - '*id': 'str', >> - 'opts': 'NetLegacyOptions' } } >> - >> -## >> -# @NetLegacyOptionsType: >> -# >> -# Since: 1.2 >> -## >> -{ 'enum': 'NetLegacyOptionsType', >> - 'data': ['none', 'nic', 'user', 'tap', 'l2tpv3', 'socket', 'vde', >> - 'bridge', 'netmap', 'vhost-user'] } > > NetLegacyOptionsType differs from NetClientDriver only by missing > 'hubport', which your code above special-cased, so on that front, the > simplification makes sense. Thanks for your review! Thomas
On 5/12/20 10:13 AM, Thomas Huth wrote: >>> +++ b/qapi/net.json >>> @@ -453,7 +453,7 @@ >>> # 'l2tpv3' - since 2.1 >>> ## >>> { 'union': 'Netdev', >>> - 'base': { 'id': 'str', 'type': 'NetClientDriver' }, >>> + 'base': { '*id': 'str', 'type': 'NetClientDriver' }, >> >> I don't think we need to make 'id' optional. > > It's required for "-net" now. But does -net generate it's own id if one is not provided? Can it still be mandatory in the QAPI type, and just figure out how to guarantee that the CLI parsing of -net provides a name early enough in the cycle to use the QAPI type without making the member optional there?
On 12/05/2020 17.51, Eric Blake wrote: > On 5/12/20 10:13 AM, Thomas Huth wrote: > >>>> +++ b/qapi/net.json >>>> @@ -453,7 +453,7 @@ >>>> # 'l2tpv3' - since 2.1 >>>> ## >>>> { 'union': 'Netdev', >>>> - 'base': { 'id': 'str', 'type': 'NetClientDriver' }, >>>> + 'base': { '*id': 'str', 'type': 'NetClientDriver' }, >>> >>> I don't think we need to make 'id' optional. >> >> It's required for "-net" now. > > But does -net generate it's own id if one is not provided? Can it still > be mandatory in the QAPI type, and just figure out how to guarantee that > the CLI parsing of -net provides a name early enough in the cycle to use > the QAPI type without making the member optional there? I guess it could be done - but we'd need to change the internal naming scheme for this. Currently, the internal id / name is created with assign_name() during qemu_net_client_setup() - which is the function that is called from the individual network backends via qemu_new_net_client() with a "model" string, e.g. net/slirp.c calls it with model="user", net/tap.c calls it with model="bridge" etc. The model string is then used in assign_name() to create the internal id (aka. name in the NetClientState). If we want to keep the "id" mandatory in the QAPI type, we have to create the internal id before calling visit_type_Netdev() in net_client_init(). But the "model" string information is not available here yet, since this takes place before the init function of the backend is called. Thus we'd need to come up with a different internal id string here, e.g. something like "__org.qemu.net-xyz" like it is done in net_param_nic() already. Do you think it is ok to change the internal name / id here? I think it should be ok - if the users care about the id of the netdevs, they should specify the ids on the command line and not rely on some internal naming schemes... do you agree? Thomas
diff --git a/net/net.c b/net/net.c index a24da66e66..4177224939 100644 --- a/net/net.c +++ b/net/net.c @@ -967,13 +967,14 @@ static int (* const net_client_init_fun[NET_CLIENT_DRIVER__MAX])( static int net_client_init1(const void *object, bool is_netdev, Error **errp) { - Netdev legacy = {0}; - const Netdev *netdev; + const Netdev *netdev = object; NetClientState *peer = NULL; if (is_netdev) { - netdev = object; - + if (!netdev->has_id) { + error_setg(errp, QERR_MISSING_PARAMETER, "id"); + return -1; + } if (netdev->type == NET_CLIENT_DRIVER_NIC || !net_client_init_fun[netdev->type]) { error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type", @@ -981,56 +982,11 @@ static int net_client_init1(const void *object, bool is_netdev, Error **errp) return -1; } } else { - const NetLegacy *net = object; - const NetLegacyOptions *opts = net->opts; - legacy.id = net->id; - netdev = &legacy; - - /* Map the old options to the new flat type */ - switch (opts->type) { - case NET_LEGACY_OPTIONS_TYPE_NONE: + if (netdev->type == NET_CLIENT_DRIVER_NONE) { return 0; /* nothing to do */ - case NET_LEGACY_OPTIONS_TYPE_NIC: - legacy.type = NET_CLIENT_DRIVER_NIC; - legacy.u.nic = opts->u.nic; - break; - case NET_LEGACY_OPTIONS_TYPE_USER: - legacy.type = NET_CLIENT_DRIVER_USER; - legacy.u.user = opts->u.user; - break; - case NET_LEGACY_OPTIONS_TYPE_TAP: - legacy.type = NET_CLIENT_DRIVER_TAP; - legacy.u.tap = opts->u.tap; - break; - case NET_LEGACY_OPTIONS_TYPE_L2TPV3: - legacy.type = NET_CLIENT_DRIVER_L2TPV3; - legacy.u.l2tpv3 = opts->u.l2tpv3; - break; - case NET_LEGACY_OPTIONS_TYPE_SOCKET: - legacy.type = NET_CLIENT_DRIVER_SOCKET; - legacy.u.socket = opts->u.socket; - break; - case NET_LEGACY_OPTIONS_TYPE_VDE: - legacy.type = NET_CLIENT_DRIVER_VDE; - legacy.u.vde = opts->u.vde; - break; - case NET_LEGACY_OPTIONS_TYPE_BRIDGE: - legacy.type = NET_CLIENT_DRIVER_BRIDGE; - legacy.u.bridge = opts->u.bridge; - break; - case NET_LEGACY_OPTIONS_TYPE_NETMAP: - legacy.type = NET_CLIENT_DRIVER_NETMAP; - legacy.u.netmap = opts->u.netmap; - break; - case NET_LEGACY_OPTIONS_TYPE_VHOST_USER: - legacy.type = NET_CLIENT_DRIVER_VHOST_USER; - legacy.u.vhost_user = opts->u.vhost_user; - break; - default: - abort(); } - - if (!net_client_init_fun[netdev->type]) { + if (netdev->type == NET_CLIENT_DRIVER_HUBPORT || + !net_client_init_fun[netdev->type]) { error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type", "a net backend type (maybe it is not compiled " "into this binary)"); @@ -1039,7 +995,7 @@ static int net_client_init1(const void *object, bool is_netdev, Error **errp) /* Do not add to a hub if it's a nic with a netdev= parameter. */ if (netdev->type != NET_CLIENT_DRIVER_NIC || - !opts->u.nic.has_netdev) { + !netdev->u.nic.has_netdev) { peer = net_hub_add_port(0, NULL, NULL); } } @@ -1143,21 +1099,13 @@ static int net_client_init(QemuOpts *opts, bool is_netdev, Error **errp) } } - if (is_netdev) { - visit_type_Netdev(v, NULL, (Netdev **)&object, &err); - } else { - visit_type_NetLegacy(v, NULL, (NetLegacy **)&object, &err); - } + visit_type_Netdev(v, NULL, (Netdev **)&object, &err); if (!err) { ret = net_client_init1(object, is_netdev, &err); } - if (is_netdev) { - qapi_free_Netdev(object); - } else { - qapi_free_NetLegacy(object); - } + qapi_free_Netdev(object); out: error_propagate(errp, err); diff --git a/qapi/net.json b/qapi/net.json index fc7c95f6d8..e344825c43 100644 --- a/qapi/net.json +++ b/qapi/net.json @@ -453,7 +453,7 @@ # 'l2tpv3' - since 2.1 ## { 'union': 'Netdev', - 'base': { 'id': 'str', 'type': 'NetClientDriver' }, + 'base': { '*id': 'str', 'type': 'NetClientDriver' }, 'discriminator': 'type', 'data': { 'nic': 'NetLegacyNicOptions', @@ -467,52 +467,6 @@ 'netmap': 'NetdevNetmapOptions', 'vhost-user': 'NetdevVhostUserOptions' } } -## -# @NetLegacy: -# -# Captures the configuration of a network device; legacy. -# -# @id: identifier for monitor commands -# -# @opts: device type specific properties (legacy) -# -# Since: 1.2 -## -{ 'struct': 'NetLegacy', - 'data': { - '*id': 'str', - 'opts': 'NetLegacyOptions' } } - -## -# @NetLegacyOptionsType: -# -# Since: 1.2 -## -{ 'enum': 'NetLegacyOptionsType', - 'data': ['none', 'nic', 'user', 'tap', 'l2tpv3', 'socket', 'vde', - 'bridge', 'netmap', 'vhost-user'] } - -## -# @NetLegacyOptions: -# -# Like Netdev, but for use only by the legacy command line options -# -# Since: 1.2 -## -{ 'union': 'NetLegacyOptions', - 'base': { 'type': 'NetLegacyOptionsType' }, - 'discriminator': 'type', - 'data': { - 'nic': 'NetLegacyNicOptions', - 'user': 'NetdevUserOptions', - 'tap': 'NetdevTapOptions', - 'l2tpv3': 'NetdevL2TPv3Options', - 'socket': 'NetdevSocketOptions', - 'vde': 'NetdevVdeOptions', - 'bridge': 'NetdevBridgeOptions', - 'netmap': 'NetdevNetmapOptions', - 'vhost-user': 'NetdevVhostUserOptions' } } - ## # @NetFilterDirection: #
Now that the "name" parameter is gone, there is hardly any difference between NetLegacy and Netdev anymore. Drop NetLegacy and always use Netdev to simplify the code quite a bit. Signed-off-by: Thomas Huth <thuth@redhat.com> --- net/net.c | 74 ++++++++------------------------------------------- qapi/net.json | 48 +-------------------------------- 2 files changed, 12 insertions(+), 110 deletions(-)