On 11/17/2015 11:38 PM, Jim Fehlig wrote:
> 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.
>
OK.
>>
>>>> + }
>>>> + }
>>>> +
>>>> 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.
>
OK.
>>
>>> 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.
Cool, Thanks for the guideline. Will send it over this new series!
Regards,
Joao
>
> Regards,
> Jim
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list