Re: [libvirt] [PATCH 2/3] xenconfig: add support for openvswitch configuration

On 11/16/18 11:26 PM, Jim Fehlig wrote: > Add support for converting openvswitch interface configuration > to/from libvirt domXML and xl.cfg(5). The xl config syntax for > virtual interfaces is described in detail in the > xl-network-configuration(5) man page. The Xen Networking wiki > also contains information and examples for using openvswitch > in xl.cfg config format > > https://wiki.xenproject.org/wiki/Xen_Networking#Open_vSwitch > > Tests are added to check conversions of openvswitch tagged and > trunked VLAN configuration. > > Signed-off-by: Jim Fehlig <jfehlig(a)suse.com&gt; > --- > src/xenconfig/xen_common.c | 113 +++++++++++++++++- > .../test-fullvirt-ovswitch-tagged.cfg | 25 ++++ > .../test-fullvirt-ovswitch-tagged.xml | 50 ++++++++ > .../test-fullvirt-ovswitch-trunked.cfg | 25 ++++ > .../test-fullvirt-ovswitch-trunked.xml | 51 ++++++++ > tests/xlconfigtest.c | 2 + > 6 files changed, 262 insertions(+), 4 deletions(-) > > diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c > index 0a9958711f..5390b933e0 100644 > --- a/src/xenconfig/xen_common.c > +++ b/src/xenconfig/xen_common.c > @@ -856,6 +856,84 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) > } > > > +static int > +xenParseVifBridge(virDomainNetDefPtr net, char *bridge) > +{ > + char *vlanstr; > + unsigned int tag; > + > + /* 'bridge' string contains a bridge name and single vlan tag */ > + vlanstr = strchr(bridge, '.'); > + if (vlanstr) { > + if (VIR_STRNDUP(net->data.bridge.brname, bridge, vlanstr - bridge) < 0) > + return -1; > + > + vlanstr++; > + if (virStrToLong_ui(vlanstr, NULL, 10, &tag) < 0) > + return -1; > + > + if (VIR_ALLOC_N(net->vlan.tag, 1) < 0) > + return -1; > + > + net->vlan.tag[0] = tag; > + net->vlan.nTags = 1; > + > + if (VIR_ALLOC(net->virtPortProfile) < 0) > + return -1; > + > + net->virtPortProfile->virtPortType = VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH; > + return 0; > + } > + > + /* 'bridge' string contains a bridge name and one or more vlan trunks */ > + vlanstr = strchr(bridge, ':'); > + if (vlanstr) { > + size_t i; > + size_t nvlans = 0; > + char **vlanstr_list = virStringSplit(bridge, ":", 0); > + > + if (!vlanstr_list) > + return -1; > + > + if (VIR_STRDUP(net->data.bridge.brname, vlanstr_list[0]) < 0) { > + virStringListFree(vlanstr_list); > + return -1; > + } > + > + for (i = 1; vlanstr_list[i]; i++) > + nvlans++; > + > + if (VIR_ALLOC_N(net->vlan.tag, nvlans) < 0) { > + virStringListFree(vlanstr_list); > + return -1; > + } > + > + for (i = 1; i <= nvlans; i++) { > + if (virStrToLong_ui(vlanstr_list[i], NULL, 10, &tag) < 0) { > + virStringListFree(vlanstr_list); > + return -1; > + } > + net->vlan.tag[i - 1] = tag; > + } > + net->vlan.nTags = nvlans; > + net->vlan.trunk = true; > + virStringListFree(vlanstr_list); > + > + if (VIR_ALLOC(net->virtPortProfile) < 0) > + return -1; > + > + net->virtPortProfile->virtPortType = VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH; > + return 0; > + } > + > + /* 'bridge' string only contains the bridge name */ > + if (VIR_STRDUP(net->data.bridge.brname, bridge) < 0) > + return -1; > + > + return 0; Not a show stopper, I just wonder if we should perhaps use: if ((vlanstr = strchr(bridge, '.'))) { ... } else if ((vlanstr = strchr(bridge, ':'))) { ... } else { ... } return 0; To me that looks a bit cleaner, but I'll leave it up to you. Don't want to be picky.