[libvirt] [PATCH 0/7] iothread api followups

[Peter Krempa]

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>