[libvirt] [PATCH 1/4] qemu: During startup need more QEMU_CAPS_OBJECT_IOTHREAD checks

[Peter Krempa]

On Tue, Oct 13, 2015 at 18:31:11 -0400, John Ferlan wrote:
> On 10/13/2015 12:08 PM, Peter Krempa wrote:
> > On Tue, Oct 13, 2015 at 11:47:07 -0400, John Ferlan wrote:
> >> During qemu process startup (qemuProcessStart), the call to
> >> qemuProcessDetectIOThreadPIDs will not attempt to fill in the
> >> ->thread_id values if the binary doesn't support IOThreads.
> >> However, subsequent calls to setup the IOThread cgroups, affinity,
> >> and scheduler parameters had no such check, thus could attempt
> >> to set thread_id = 0, which would not be a good idea.
> >>
> >> Signed-off-by: John Ferlan <jferlan at redhat.com>
> >> ---
> >>  src/qemu/qemu_cgroup.c  |  3 +++
> >>  src/qemu/qemu_process.c | 18 ++++++++++++------
> >>  2 files changed, 15 insertions(+), 6 deletions(-)
> > 
> > Do we even allow to start a VM that has IOthreads enabled but qemu does
> > not support them? In that case that's the point to fix so that we have a
> > central point where it's checked.
> > 
> 
> Upon further reflection...
> 
> During qemuBuildCommandLine there is a check for 'iothreads > 0 and have
> the capability', then create 'niothreadids' objects.
> 
> Then during qemuCheckIOThreads called from qemuBuildDriveDevStr, we will
> fail if there's a disk with an "iothread='#'" property.
> 
> Thus, if just defined (eg, <iothreads> and <iothreadids>), we ignore
> them if the emulator doesn't support it. However, if used, then yes we

That seems rather strange. With other things we usually point out that
the configuration is broken so that the user knows that the features are
not available.

Additionally, since you ignore creation of the threads if qemu doesn't
support it you might get into situations where if you migrate to a newer
version you actually will create a different configuration with the same
config. It might not fail for iothreads, but it's calling for problems.

> fail. Naturally this is one of those only one downstream platform really
> cares - just happens to be one that does a lot of QA for Red Hat.
> 
> Although a fairly useless configuration, I think because it seems it's
> possible to have 'niothreadids' with empty fields other than
> 'iothread_id' that checking for the capability is more correct in these
> instances than checking for niothreadids.
> 
> Unless of course you feel we should fail in qemuBuildCommandLine if
> iothreads are defined.

I do feel rather strongly that we should point out that the config is
invalid rather than just silently ignoring stuff the user requested.

Peter

> 
> John
> >>
> >> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c

[please trim the rest of the message you are not responding to]
-------------- 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/20151014/64250287/attachment-0001.sig>