...
>>>> --- a/src/qemu/qemu_process.c
>>>> +++ b/src/qemu/qemu_process.c
>>>> @@ -2267,12 +2267,16 @@ qemuProcessDetectIOThreadPIDs(virQEMUDriverPtr driver,
>>>> goto cleanup;
>>>> }
>>>
>>> A few lines prior here is the check that the expected thread count
>>> equals to the actual thread count. For some reason a few lines before
>>> returns success if 0 threads are returned by the monitor. The two checks
>>> should be inverted so that it makes sense.
>>>
>>
>> If there are no threads, then it's not a failure, thus change ret to be
>> 0. Again, this is something that's not within the scope of this set of
>> changes and I believe if necessary could be a followup patch.
>>
>> I'm not clear on the value of changing the order of the checks.
>
> The problem is that if there are no iothreads reported by qemu, but we
> did request some then it IS failure.
>
But that's an issue not related to iothreadid's per se - it's a more
common general issue that should be a follow-up patch then I think.
That is not introduced by this set of changes.
Other issues were addressed changed - do you need to see the diffs or an
updated patch with the diffs already squashed in?
John
>>
>> Check failure first - return failure
>>
>> Check possible successes next.
>>
>>>>
>>>> - if (VIR_ALLOC_N(priv->iothreadpids, niothreads) < 0)
>>>> - goto cleanup;
>>>> - priv->niothreadpids = niothreads;
>>>> + for (i = 0; i < vm->def->niothreadids; i++) {
>>>> + unsigned int iothread_id;
>>>>
>>>> - for (i = 0; i < priv->niothreadpids; i++)
>>>> - priv->iothreadpids[i] = iothreads[i]->thread_id;
>>>> + if (qemuDomainParseIOThreadAlias(iothreads[i]->name,
>>>> + &iothread_id) < 0)
>>>> + goto cleanup;
>>>> +
>>>> + vm->def->iothreadids[i]->thread_id = iothreads[i]->thread_id;
>>>> + vm->def->iothreadids[i]->iothread_id = iothread_id;
>>>
>>> This construct is wrong since it expects that the order the devices are
>>> stored in libvirt is the same as in the qemu monitor reply. You need to
>>> iterate the list of threads from the monitor and lookup the
>>> corresponding info for it.
>>
>> Ahh... Right, but we are getting the iothread_id's here from the monitor
>> and filling in an array - the call to virDomainIOThreadIDFind had better
>> not fail ;-) - if does though the domain disappears, so which is worse?
>>
>> So ...
>>
>> virDomainIOThreadIDDefPtr iothrid;
>> ...
>>
>> if (!(iothrid = virDomainIOThreadIDFind(vm->def, iothread_id))) {
>> virReportError(VIR_ERR_INVALID_ARG,
>> _("iothread %d not found"), iothread_id);
>> }
>> iothrid->thread_id = iothreads[i]->thread_id;
>
> Yep.
>
>>
>>>
>>> Btw, it would be much easier if the monitor code would parse the IDs
>>> since you wouldn't need to parse them here (I've already suggested this
>>> once).
>>>
>>
>> Again, design/structure change - can we let this be a followup patch?
>
> Yes this can be a separate change. I only find it less wasteful since
> every single caller needs to parse the IDs anyways.
>
>>
>>>> + }
>>>>
>>>> ret = 0;
>>>>
>>>
>>> ...
>>>
>
> Peter
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list