Re: [PATCH 0/7] iothread api followups

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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.

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
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.


John



--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]