On Wed, Mar 25, 2015 at 15:00:58 -0400, John Ferlan wrote: > > > On 03/25/2015 02:39 PM, Ján Tomko wrote: > > Looking at the proposed SetIOThread API, I noticed > > that the virsh command for printing the info is named 'iothreadsinfo'. > > This seemed counter-intuitive for me. > > > > Since the API has not been released yet, I propose a rename of the command to: > > iothreadinfo (patch 5) > > and the API for freeing one struct to: > > virDomainIOThreadInfoFree (patch 1) > > > > I don't feel as strongly about renaming the virDomainGetIOThreadsInfo API > > (patches 3, 4) or the internal APIs (patch 2). > > > > Looking at virVcpuInfoPtr (which might not be the best inspiration), > > I realized including the cpu time might be a good idea. > > > > Ján Tomko (7): > > Rename virDomainIOThreadsInfoFree to virDomainIOThreadInfoFree > > Rename qemuMonitorIOThreadsInfo* to qemuMonitorIOThreadInfo* > > Rename DomainGetIOThreadsInfo to DomainGetIOThreadInfo > > gendispatch: remove IOThreads from name fixups > > virsh: rename iothreadsinfo to iothreadinfo > > Do not use vshPrintPinInfo in iothreadinfo > > add cpu time to iothreadinfo > > > > daemon/remote.c | 15 +++++++------ > > include/libvirt/libvirt-domain.h | 5 +++-- > > src/driver-hypervisor.h | 4 ++-- > > src/libvirt-domain.c | 20 ++++++++--------- > > src/libvirt_public.syms | 4 ++-- > > src/qemu/qemu_driver.c | 24 ++++++++++++++------ > > src/qemu/qemu_monitor.c | 4 ++-- > > src/qemu/qemu_monitor.h | 10 ++++----- > > src/qemu/qemu_monitor_json.c | 8 +++---- > > src/qemu/qemu_monitor_json.h | 2 +- > > src/qemu/qemu_process.c | 4 ++-- > > src/remote/remote_driver.c | 23 ++++++++++---------- > > src/remote/remote_protocol.x | 11 +++++----- > > src/remote_protocol-structs | 7 +++--- > > src/rpc/gendispatch.pl | 1 - > > tests/qemumonitorjsontest.c | 4 ++-- > > tools/virsh-domain.c | 47 ++++++++++++++++++++++++++++------------ > > 17 files changed, 113 insertions(+), 80 deletions(-) > > > > > Didn't dig into all the details, but use of IOThreads v IOThread was > mostly an artifact of the technology name. I'm ambivalent to the naming > issue. You'll have to have follow-ups in libvirt-python and > libvirt-perl though. Sometimes the *s* is relevant though - as in we're > returning all IOThreads found vs returning just one IOThread > > w/r/t virsh iothreadsinfo vs. iothreadinfo - I see the latter as > displaying just one rather than multiple. > > w/r/t patch 6 - does the output change? IOW: The current way displays > "1", "3", "0-3", etc. Does the method you're proposing change to the > "y---", "--y-", "yyyy" etc. method? I prefer the former.... The commit > message would need to list the change if there is one. The output does not change. In fact I think the virsh reimpl of virBitmapFormat should be killed and replaced with the function we already have. > > w/r/t patch 7 - while it's a "nice to have" I think its far more > relevant to list the Resource(s) associated with the thread than the CPU > time used. Listing the Resource was rejected in a much earlier patch Actually, the "resources" is a configruation option and I don't see a way to change it unless you unplug the disk while CPU time is a statistics function that changes a lot. > review, so I don't see why listing the CPU time is important and failing > in qemuDomainGetIOThreadsLive because we cannot get the that time, but > yet deciding later on to not print it if it doesn't exist doesn't make > total sense. If the VM is offline, then the CPU time is obviously not filled, while when it's live we should return it. Peter
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list