[libvirt] [PATCH] network: don't allow multiple default portgroups

[Laine Stump]

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.