[libvirt] [PATCH V2 1/2] libxl: support interface type=network

[Jim Fehlig]

Laine Stump wrote:
> On 06/11/2014 12:25 AM, Jim Fehlig wrote:
>   
>> Add support for <interface type='network'> in the libxl driver.
>>
>> Signed-off-by: Jim Fehlig <jfehlig at suse.com>
>> ---
>>
>> V2:
>> Support both libvirt's traditional managed networks and libvirt
>> networks that use an existing host bridge.
>>
>>  src/libxl/libxl_conf.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 48 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>> index cec37d6..9c453d8 100644
>> --- a/src/libxl/libxl_conf.c
>> +++ b/src/libxl/libxl_conf.c
>> @@ -874,6 +874,7 @@ libxlMakeNic(virDomainDefPtr def,
>>               libxl_device_nic *x_nic)
>>  {
>>      bool ioemu_nic = STREQ(def->os.type, "hvm");
>> +    virDomainNetType actual_type = virDomainNetGetActualType(l_nic);
>>  
>>      /* TODO: Where is mtu stored?
>>       *
>> @@ -899,16 +900,60 @@ libxlMakeNic(virDomainDefPtr def,
>>      if (VIR_STRDUP(x_nic->ifname, l_nic->ifname) < 0)
>>          return -1;
>>  
>> -    switch (l_nic->type) {
>> +    switch (actual_type) {
>>          case VIR_DOMAIN_NET_TYPE_BRIDGE:
>> -            if (VIR_STRDUP(x_nic->bridge, l_nic->data.bridge.brname) < 0)
>> +            if (VIR_STRDUP(x_nic->bridge,
>> +                           virDomainNetGetActualBridgeName(l_nic)) < 0)
>>                  return -1;
>>              /* fallthrough */
>>          case VIR_DOMAIN_NET_TYPE_ETHERNET:
>>              if (VIR_STRDUP(x_nic->script, l_nic->script) < 0)
>>                  return -1;
>>              break;
>> -        default:
>> +        case VIR_DOMAIN_NET_TYPE_NETWORK:
>> +        {
>> +            bool fail = false;
>> +            char *brname = NULL;
>> +            virNetworkPtr network;
>> +            virConnectPtr conn;
>> +            virErrorPtr errobj;
>> +
>> +            if (!(conn = virConnectOpen("xen:///system")))
>> +                return -1;
>>     
>
> (The conn is necessary in order to call the functions that retrieve the
> bridge device name for the network)
>
> One difference in the way that qemu deals with this is that in most
> cases the higher level function already has a conn ptr, and that is
> passed down the call stack to the user. There is one case where this
> doesn't happen - the autostart of a domain. In that case, qemu calls
> virConnectOpen(cfg->uri) to get a conn.
>   

Right, and that depends on whether running privileged or not.  From
qemu_conf.c, virQEMUDriverConfigNew:

    cfg->uri = privileged ? "qemu:///system" : "qemu:///session";

> In this case, you are always creating a new conn, even when one may
> already exist a few layers up the call stack,

I originally looked into passing conn to libxlMakeNic, but it is
actually quite a bit of code refactoring.  I think that should be done
in conjunction with decomposing libxlDomainStart a bit.  Currently that
function builds the domain config and handles the details of starting
the domain.  I'd like to pull the build part out, allowing a cleaner
approach to passing conn where needed.  E.g. I could see conn being used
in libxlMakeDisk some day.

>  and the conn you create is
> using the hardcoded "xen:///system" rather than taking something from
> config.
>
> As long as this is okay with you, it's okay with me. I just wonder about
> two things:
>   

I think it is okay.

> 1) does xen support non-privileged libvirt, and if so what would happen
> in this case (I suppose it would just fail due to lack of authorization).
>   

Only supports privileged.

> 2) is the uri configurable anywhere, as it is for libvirt?
>   

Do you mean s/libvirt/the qemu driver/ ?  As mentioned above, cfg->uri
depends on privileged in the qemu driver?  For the libxl driver,
privileged is always true.

>
>   
>> +
>> +            if (!(network =
>> +                  virNetworkLookupByName(conn, l_nic->data.network.name))) {
>> +                virObjectUnref(conn);
>> +                return -1;
>> +            }
>> +
>> +            if ((brname = virNetworkGetBridgeName(network))) {
>> +                if (VIR_STRDUP(x_nic->bridge, brname) < 0)
>> +                    fail = true;
>> +            } else {
>> +                fail = true;
>> +            }
>> +
>> +            VIR_FREE(brname);
>> +
>> +            /* Preserve any previous failure */
>> +            errobj = virSaveLastError();
>> +            virNetworkFree(network);
>> +            virSetError(errobj);
>> +            virFreeError(errobj);
>> +            virObjectUnref(conn);
>> +            if (fail)
>> +                return -1;
>> +            break;
>> +        }
>> +        case VIR_DOMAIN_NET_TYPE_USER:
>> +        case VIR_DOMAIN_NET_TYPE_SERVER:
>> +        case VIR_DOMAIN_NET_TYPE_CLIENT:
>> +        case VIR_DOMAIN_NET_TYPE_MCAST:
>> +        case VIR_DOMAIN_NET_TYPE_INTERNAL:
>> +        case VIR_DOMAIN_NET_TYPE_DIRECT:
>> +        case VIR_DOMAIN_NET_TYPE_HOSTDEV:
>> +        case VIR_DOMAIN_NET_TYPE_LAST:
>>              virReportError(VIR_ERR_INTERNAL_ERROR,
>>                      _("libxenlight does not support network device type %s"),
>>                      virDomainNetTypeToString(l_nic->type));
>>     
>
> ACK, pending the answers to the above questions.
>   

Thanks for asking the questions, which made me rethink this and feel
confident it is okay :-).  I'll push this, along with 2/2.

Regards,
Jim