Re: [libvirt] [PATCH v5 02/10] qemu: Use domain iothreadids to IOThread's 'thread_id'

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

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