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@xxxxxxxx>
>> ---
>>
>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list