[libvirt] [PATCH RFC] libxl: reverse defaults on HVM net device attach

Jim Fehlig jfehlig at suse.com
Tue Dec 13 16:07:20 UTC 2016


Joao Martins wrote:
> On 12/13/2016 03:24 AM, Jim Fehlig wrote:
>> On 12/09/2016 04:35 AM, Joao Martins wrote:
>>> libvirt libxl picks its own default with respect to the default NIC
>>> to use. libxlMakeNic is the one responsible for this and on boot it
>>> picks LIBXL_NIC_TYPE_VIF_IOEMU such that it accomodates both PV and
>>> emulated one. The good behaving guest at boot will then select the pv
>>> and unplug the emulated device.
>>>
>>> Now, on HVM when attaching an interface it will pick the same default
>>> that is LIBXL_NIC_TYPE_VIF_IOEMU which as a result will fail the attach
>>> (see xen commit 32e9d0f ("libxl: nic type defaults to vif in hotplug for
>>> hvm guest"). Xen doesn't yet support the hotplug of emulated devices,
>>> but we don't want to rule out that case either, which might get support
>>> in the future. Hence we simply reverse the defaults when we are
>>> attaching the interface which allows libvirt to prefer the PV nic first
>>> without adding "model='netfront'" following the same pattern as above
>>> commit. Also to avoid ruling out the emulated one we set to
>>> LIBXL_NIC_TYPE_IOEMU when setting a model type that is not 'netfront'.
>>>
>>> Signed-off-by: Joao Martins <joao.m.martins at oracle.com>
>>> ---
>>>
>>> This allows Openstack to attach network interfaces, which currently
>>> is broken on libxl if it doesn't set model 'netfront'. I am not sure
>>> whether this is the best way (hence RFC) or if users should really be
>>> setting the model='netfront' when attaching devices. But sounds to me
>>> that it would be better to have by default the supported, and if users
>>> want emulated attach to specify a nic model. Thoughts?
>> Agreed. I think it is best to mimic the behavior of xl/libxl.
> Cool, Thanks for the feedback.
> 
>>> ---
>>>  src/libxl/libxl_conf.c   | 9 ++++++---
>>>  src/libxl/libxl_conf.h   | 3 ++-
>>>  src/libxl/libxl_driver.c | 2 +-
>>>  3 files changed, 9 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>>> index dcf8e7e..50aa958 100644
>>> --- a/src/libxl/libxl_conf.c
>>> +++ b/src/libxl/libxl_conf.c
>>> @@ -881,9 +881,10 @@ libxlMakeDiskList(virDomainDefPtr def, libxl_domain_config *d_config)
>>>  int
>>>  libxlMakeNic(virDomainDefPtr def,
>>>               virDomainNetDefPtr l_nic,
>>> -             libxl_device_nic *x_nic)
>>> +             libxl_device_nic *x_nic,
>>> +             bool attach)
>>>  {
>>> -    bool ioemu_nic = def->os.type == VIR_DOMAIN_OSTYPE_HVM;
>>> +    bool ioemu_nic = def->os.type == VIR_DOMAIN_OSTYPE_HVM && !attach;
>>>      virDomainNetType actual_type = virDomainNetGetActualType(l_nic);
>>>      virNetworkPtr network = NULL;
>>>      virConnectPtr conn = NULL;
>>> @@ -917,6 +918,8 @@ libxlMakeNic(virDomainDefPtr def,
>>>              goto cleanup;
>>>          if (STREQ(l_nic->model, "netfront"))
>>>              x_nic->nictype = LIBXL_NIC_TYPE_VIF;
>>> +        else
>>> +            x_nic->nictype = LIBXL_NIC_TYPE_VIF_IOEMU;
>>>      }
>>>
>>>      if (VIR_STRDUP(x_nic->ifname, l_nic->ifname) < 0)
>> This logic has gotten a bit confusing for the casual reader not familiar with 
>> the Xen behavior. It is late here and my eyes are getting heavy, but what do you
>> think about replacing this hunk with something like the below? Feel free to 
>> suggest something more clear and succinct, but at minimum I think a comment 
>> describing the Xen behavior is warranted.
> The patch looks good, just two minor comments below. I can submit another
> version, and as I am taking your changes - may I add your SoB tag?

Sure, no problem.

> 
>> Regards,
>> Jim
>>
>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>> index dcf8e7e..6650b21 100644
>> --- a/src/libxl/libxl_conf.c
>> +++ b/src/libxl/libxl_conf.c
>> @@ -881,7 +881,8 @@ libxlMakeDiskList(virDomainDefPtr def, libxl_domain_config 
>> *d_config)
>>   int
>>   libxlMakeNic(virDomainDefPtr def,
>>                virDomainNetDefPtr l_nic,
>> -             libxl_device_nic *x_nic)
>> +             libxl_device_nic *x_nic,
>> +             bool attach)
>>   {
>>       bool ioemu_nic = def->os.type == VIR_DOMAIN_OSTYPE_HVM;
>              ^^^^^^^^^ This is no longer needed after this chunk.

Right. I guess the secret is out that I didn't even compile-test my suggestion :-).

> 
>>       virDomainNetType actual_type = virDomainNetGetActualType(l_nic);
>> @@ -907,16 +908,33 @@ libxlMakeNic(virDomainDefPtr def,
>>
>>       virMacAddrGetRaw(&l_nic->mac, x_nic->mac);
>>
>> -    if (ioemu_nic)
>> -        x_nic->nictype = LIBXL_NIC_TYPE_VIF_IOEMU;
>> -    else
>> -        x_nic->nictype = LIBXL_NIC_TYPE_VIF;
>> -
>> +    /*
>> +     * The nictype field of libxl_device_nic structure tells Xen which type of
>> +     * NIC device to create for the domain. LIBXL_NIC_TYPE_VIF specifies a
>> +     * PV NIC. LIBXL_NIC_TYPE_VIF_IOEMU specifies a PV and emulated NIC,
>> +     * allowing the domain to choose which NIC to use and unplug the unused
>> +     * one. LIBXL_NIC_TYPE_VIF_IOEMU is only valid for HVM domains. Further,
>> +     * if hotplugging the NIC, emulated NICs are currently not supported.
>> +     */
> Probably in addition would it worth mentioning we mimic xl/libxl behavior, like
> below comment?

Nod. PV vs PV+emulated NIC is a Xen oddity that has caused lots of confusion
over the years, so I think it is hard to be too verbose here.

> 
> "Alternatively one could set LIBXL_NIC_TYPE_UNKNOWN and let libxl decide, but
> its behaviour might not be consistent across all supported xen versions. The
> other nictype values are well established already, hence we manually select our
> own default and mimic xl/libxl behaviour since xen commit 32e9d0f."
> 
>>       if (l_nic->model) {
>> +        if (def->os.type == VIR_DOMAIN_OSTYPE_XEN &&
>> +            STRNEQ(l_nic->model, "netfront")) {
>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> +                           _("only model 'netfront' is supported for "
>> +                             "Xen PV domains"));
>> +            return -1;
>> +        }
>>           if (VIR_STRDUP(x_nic->model, l_nic->model) < 0)
>>               goto cleanup;
>>           if (STREQ(l_nic->model, "netfront"))
>>               x_nic->nictype = LIBXL_NIC_TYPE_VIF;
>> +        else
>> +            x_nic->nictype = LIBXL_NIC_TYPE_VIF_IOEMU;
>> +    } else {
>> +        if (def->os.type == VIR_DOMAIN_OSTYPE_HVM && !attach)
>> +            x_nic->nictype = LIBXL_NIC_TYPE_VIF_IOEMU;
>> +        else
>> +            x_nic->nictype = LIBXL_NIC_TYPE_VIF;
>>       }
>>
>>       if (VIR_STRDUP(x_nic->ifname, l_nic->ifname) < 0)
>>
> 
> Other than that, Looks good. One suggestion would be to group the tails of
> the if/else, but the condition would have some redundancy in the predicates,
> hence this one looks clearer.

Feel free to rearrange that logic if you can think of something more clever. My
main review note about your patch was the need for a comment describing the Xen
behavior.

Regards,
Jim




More information about the libvir-list mailing list