[libvirt] [PATCH 0/7] iothread api followups
Peter Krempa
pkrempa at redhat.com
Thu Mar 26 10:22:52 UTC 2015
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150326/1fe9d61d/attachment-0001.sig>
More information about the libvir-list
mailing list