[libvirt] [PATCH] network: don't allow multiple default portgroups
This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=868483 virNetworkUpdate, virNetworkDefine, and virNetworkCreate all three allow network definitions to contain multiple <portgroup> elements with default='yes'. Only a single default portgroup should be allowed for each network. This patch updates networkValidate() (called by both virNetworkCreate() and virNetworkDefine()) and virNetworkDefUpdatePortGroup (called by virNetworkUpdate() to not allow multiple default portgroups. --- src/conf/network_conf.c | 35 ++++++++++++++++++++++++++--------- src/network/bridge_driver.c | 12 ++++++++++++ 2 files changed, 38 insertions(+), 9 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 2f9ad2e..8976f2a 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2752,7 +2752,8 @@ virNetworkDefUpdatePortGroup(virNetworkDefPtr def, /* virNetworkUpdateFlags */ unsigned int fflags ATTRIBUTE_UNUSED) { - int ii, ret = -1; + int ii, foundName = -1, foundDefault = -1; + int ret = -1; virPortGroupDef portgroup; memset(&portgroup, 0, sizeof(portgroup)); @@ -2766,9 +2767,11 @@ virNetworkDefUpdatePortGroup(virNetworkDefPtr def, /* check if a portgroup with same name already exists */ for (ii = 0; ii < def->nPortGroups; ii++) { if (STREQ(portgroup.name, def->portGroups[ii].name)) - break; + foundName = ii; + if (def->portGroups[ii].isDefault) + foundDefault = ii; } - if (ii == def->nPortGroups && + if (foundName == -1 && ((command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) || (command == VIR_NETWORK_UPDATE_COMMAND_DELETE))) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -2776,7 +2779,7 @@ virNetworkDefUpdatePortGroup(virNetworkDefPtr def, "in network '%s' matching <portgroup name='%s'>"), def->name, portgroup.name); goto cleanup; - } else if (ii < def->nPortGroups && + } else if (foundName >= 0 && ((command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) || (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST))) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -2787,11 +2790,25 @@ virNetworkDefUpdatePortGroup(virNetworkDefPtr def, goto cleanup; } + /* if there is already a different default, we can't make this + * one the default. + */ + if (command != VIR_NETWORK_UPDATE_COMMAND_DELETE && + portgroup.isDefault && + foundDefault >= 0 && foundDefault != foundName) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("a different portgroup entry in " + "network '%s' is already set as the default. " + "Only one default is allowed."), + def->name); + goto cleanup; + } + if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) { /* replace existing entry */ - virPortGroupDefClear(&def->portGroups[ii]); - def->portGroups[ii] = portgroup; + virPortGroupDefClear(&def->portGroups[foundName]); + def->portGroups[foundName] = portgroup; memset(&portgroup, 0, sizeof(portgroup)); } else if ((command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) || @@ -2816,9 +2833,9 @@ virNetworkDefUpdatePortGroup(virNetworkDefPtr def, } else if (command == VIR_NETWORK_UPDATE_COMMAND_DELETE) { /* remove it */ - virPortGroupDefClear(&def->portGroups[ii]); - memmove(def->portGroups + ii, def->portGroups + ii + 1, - sizeof(*def->portGroups) * (def->nPortGroups - ii - 1)); + virPortGroupDefClear(&def->portGroups[foundName]); + memmove(def->portGroups + foundName, def->portGroups + foundName + 1, + sizeof(*def->portGroups) * (def->nPortGroups - foundName - 1)); def->nPortGroups--; ignore_value(VIR_REALLOC_N(def->portGroups, def->nPortGroups)); } else { diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 1c97f29..8837843 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2618,6 +2618,7 @@ networkValidate(virNetworkDefPtr def) { int ii; bool vlanUsed, vlanAllowed; + const char *defaultPortGroup = NULL; /* The only type of networks that currently support transparent * vlan configuration are those using hostdev sr-iov devices from @@ -2638,6 +2639,17 @@ networkValidate(virNetworkDefPtr def) == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH)) { vlanAllowed = true; } + if (def->portGroups[ii].isDefault) { + if (defaultPortGroup) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("network '%s' has multiple default " + "<portgroup> elements (%s and %s), " + "but only one default is allowed"), + def->name, defaultPortGroup, + def->portGroups[ii].name); + } + defaultPortGroup = def->portGroups[ii].name; + } } if (vlanUsed && !vlanAllowed) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, -- 1.7.11.7
On 10/20/2012 02:45 AM, Laine Stump wrote:
This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=868483
virNetworkUpdate, virNetworkDefine, and virNetworkCreate all three allow network definitions to contain multiple <portgroup> elements with default='yes'. Only a single default portgroup should be allowed for each network.
This patch updates networkValidate() (called by both virNetworkCreate() and virNetworkDefine()) and virNetworkDefUpdatePortGroup (called by virNetworkUpdate() to not allow multiple default portgroups. --- src/conf/network_conf.c | 35 ++++++++++++++++++++++++++--------- src/network/bridge_driver.c | 12 ++++++++++++ 2 files changed, 38 insertions(+), 9 deletions(-)
ACK.
@@ -2787,11 +2790,25 @@ virNetworkDefUpdatePortGroup(virNetworkDefPtr def, goto cleanup; }
+ /* if there is already a different default, we can't make this + * one the default. + */ + if (command != VIR_NETWORK_UPDATE_COMMAND_DELETE && + portgroup.isDefault && + foundDefault >= 0 && foundDefault != foundName) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("a different portgroup entry in " + "network '%s' is already set as the default. " + "Only one default is allowed."),
How does one change which group is the default? Via back-to-back change commands, the first which removes the default flag from the old name, and the second adding the new name with the new default flag? I think that works, so it doesn't stop this patch. However, I wonder if you want a followup patch to add a flag to the overall API that allows one to forcefully change the default to the new element by also removing the default flag from the old group, all in one API call. It would be needed if there is ever a reason where having a window with no default is unacceptable, so atomically moving the default in one API call becomes important to close such a window, but I'm having a hard time convincing myself whether we even care about such a race. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10/20/2012 12:49 PM, Eric Blake wrote:
On 10/20/2012 02:45 AM, Laine Stump wrote:
This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=868483
virNetworkUpdate, virNetworkDefine, and virNetworkCreate all three allow network definitions to contain multiple <portgroup> elements with default='yes'. Only a single default portgroup should be allowed for each network.
This patch updates networkValidate() (called by both virNetworkCreate() and virNetworkDefine()) and virNetworkDefUpdatePortGroup (called by virNetworkUpdate() to not allow multiple default portgroups. --- src/conf/network_conf.c | 35 ++++++++++++++++++++++++++--------- src/network/bridge_driver.c | 12 ++++++++++++ 2 files changed, 38 insertions(+), 9 deletions(-) ACK.
@@ -2787,11 +2790,25 @@ virNetworkDefUpdatePortGroup(virNetworkDefPtr def, goto cleanup; }
+ /* if there is already a different default, we can't make this + * one the default. + */ + if (command != VIR_NETWORK_UPDATE_COMMAND_DELETE && + portgroup.isDefault && + foundDefault >= 0 && foundDefault != foundName) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("a different portgroup entry in " + "network '%s' is already set as the default. " + "Only one default is allowed."), How does one change which group is the default? Via back-to-back change commands, the first which removes the default flag from the old name, and the second adding the new name with the new default flag? I think that works, so it doesn't stop this patch.
However, I wonder if you want a followup patch to add a flag to the overall API that allows one to forcefully change the default to the new element by also removing the default flag from the old group, all in one API call. It would be needed if there is ever a reason where having a window with no default is unacceptable, so atomically moving the default in one API call becomes important to close such a window, but I'm having a hard time convincing myself whether we even care about such a race.
Good point, and a good proposal for how to close it. I'm also not sure if that will ever be important (to date I've only found two people who actually even *use* portgroups so far (one of them just yesterday), but we should probably do that just to avoid future surprises. I'm pushing this patch as-is, though, and adding your suggestion to my to-do list.
participants (2)
-
Eric Blake -
Laine Stump