[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 3/8] libxl: implement virDomainInterfaceStats
Joao Martins wrote: > > On 11/17/2015 02:48 AM, Jim Fehlig wrote: >> On 11/13/2015 06:14 AM, Joao Martins wrote: >>> Introduce support for domainInterfaceStats API call for querying >>> network interface statistics. Consequently it also enables the >>> use of `virsh domifstat <dom> <interface name>` command. >>> >>> After succesful guest creation we fill the network >>> interfaces names based on domain, device id and append suffix >>> if it's emulated in the following form: vif<domid>.<devid>[-emu]. Because >>> we need the devid from domain config (which is filled by libxl on domain >>> create) we cannot do generate the names in console callback. >> Bummer, but see below. >> >>> On domain >>> cleanup we also clear ifname, in case it was set by libvirt (i.e. >>> being prefixed with "vif"). We also skip these two steps in case the name >>> of the interface was manually inserted by the adminstrator. >>> >>> For getting the interface statistics we resort to virNetInterfaceStats >>> and let libvirt handle the platform specific nits. Note that the latter >>> is not yet supported in FreeBSD. >>> >>> Signed-off-by: Joao Martins <joao.m.martins@xxxxxxxxxx> >>> --- >>> Changes since v2: >>> - Clear ifname if it's autogenerated, since otherwise will persist >>> on successive domain starts. Change commit message reflecting this >>> change. >>> >>> Changes since v1: >>> - Fill <virDomainNetDef>.ifname after domain start with generated >>> name from libxl based on domain id and devid returned by libxl. >>> After that path validation don interfaceStats is enterily based >>> on ifname pretty much like the other drivers. >>> - Modify commit message reflecting the changes mentioned in >>> the previous item. >>> - Bump version to 1.2.22 >>> --- >>> src/libxl/libxl_domain.c | 26 ++++++++++++++++++++++++++ >>> src/libxl/libxl_driver.c | 48 >>> ++++++++++++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 74 insertions(+) >>> >>> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c >>> index 40dcea1..adb4563 100644 >>> --- a/src/libxl/libxl_domain.c >>> +++ b/src/libxl/libxl_domain.c >>> @@ -728,6 +728,17 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, >>> } >>> } >>> >>> + if ((vm->def->nnets)) { >>> + ssize_t i; >>> + >>> + for (i = 0; i < vm->def->nnets; i++) { >>> + virDomainNetDefPtr net = vm->def->nets[i]; >>> + >>> + if (STRPREFIX(net->ifname, "vif")) >>> + VIR_FREE(net->ifname); >> Would not be nice if user-specified ifname started with "vif" :-). >> > I think QEMU they do in a similar way, in case, specified interfaces started > with a prefix that is solely for autogenerated interface naming. Ah, right. Same with bhyve and UML drivers. > Would you > prefer removing it? The problem with removing this is that ifname would > persist > on the XML, and future domainStart would use the old value. Yep, understood. Fine to leave it. > >>> + } >>> + } >>> + >>> if (virAsprintf(&file, "%s/%s.xml", cfg->stateDir, vm->def->name) > 0) >>> { >>> if (unlink(file) < 0 && errno != ENOENT && errno != ENOTDIR) >>> VIR_DEBUG("Failed to remove domain XML for %s", vm->def->name); >>> @@ -901,6 +912,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, >>> virDomainObjPtr vm, >>> virDomainDefPtr def = NULL; >>> virObjectEventPtr event = NULL; >>> libxlSavefileHeader hdr; >>> + ssize_t i; >>> int ret = -1; >>> uint32_t domid = 0; >>> char *dom_xml = NULL; >>> @@ -1023,6 +1035,20 @@ libxlDomainStart(libxlDriverPrivatePtr driver, >>> virDomainObjPtr vm, >>> */ >>> vm->def->id = domid; >>> >>> + for (i = 0; i < vm->def->nnets; i++) { >>> + virDomainNetDefPtr net = vm->def->nets[i]; >>> + libxl_device_nic *x_nic = &d_config.nics[i]; >>> + const char *suffix = >>> + x_nic->nictype != LIBXL_NIC_TYPE_VIF ? "-emu" : ""; >>> + >>> + if (net->ifname) >>> + continue; >>> + >>> + if (virAsprintf(&net->ifname, "vif%d.%d%s", >>> + domid, x_nic->devid, suffix) < 0) >>> + continue; >>> + } >>> + >> This could be done in the callback, if we added the libxl_domain_config >> object >> to the libxlDomainObjPrivate struct. Currently we create, use, and dispose >> the >> object in libxlDomainStart, but it would probably be useful keep the object >> around while the domain is active. > That's a good idea. This chunk would definitely be better placed in > ConsoleCallback. Agreed, with ConsoleCallback being renamed to something a bit more generic like DomainStartCallback or similar. > >> Actually, you could directly use the object >> in libxlDomainInterfaceStats if it was included in the libxlDomainObjPrivate >> struct. I realize it is a bit more work than this or the V1 approach, but >> what >> do you think about it? > Huum, IIUC we would be doing similar to V1, with the difference that we would > do > it more reliably by knowning devid in advance through usage of > libxl_domain_config. The good thing about this version though is that it > simplifies things simple for interfaceStats and getAllDomainStats, since all > it > takes is just to compare against the interface name we calculated beforehand > on > domain start. Moreover we can actually see what interfaces each domain has > when > doing domiflist as a result of setting net->ifname (right now only "-" would > show up in the name). Yes, good point. I think it is best to generate the name when starting the VM. Other drivers take the same approach. So it looks like this simple patch could become a series in itself: one patch adding libxl_domain_config to the libxlDomainObjPrivate struct, another patch to rename ConsoleCallback to a more generic name, and finally this patch implementing virDomainInterfaceStats on top of the others. Regards, Jim _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |