Re: [libvirt] [PATCH REBASE 4/5] qemu: fix domain object wait to handle monitor errors

On 05/03/2018 03:54 AM, Nikolay Shirokovskiy wrote: > > > On 01.05.2018 01:03, John Ferlan wrote: >> >> >> On 04/18/2018 10:44 AM, Nikolay Shirokovskiy wrote: >>> Block job abort operation can not handle properly qemu crashes >>> when waiting for abort/pivot completion. Deadlock scenario is next: >>> >>> - qemuDomainBlockJobAbort waits for pivot/abort completion >>> - qemu crashes, then qemuProcessBeginStopJob broadcasts for VM condition and >>> then waits for job condition (taken by qemuDomainBlockJobAbort) >>> - qemuDomainBlockJobAbort awakes but nothing really changed, VM is still >>> active (vm->def->id != -1) so thread starts waiting for completion again. >>> Now two threads are in deadlock. >>> >>> First let's add some condition besides domain active status so that waiting >>> thread can check it when awakes. Second let's move signalling to the place >>> where condition is set, namely monitor eof/error handlers. Having signalling >>> in qemuProcessBeginStopJob is not useful. >>> >>> The patch copies monitor error to domain state because at time >>> waiting thread awakes there can be no monitor and it is useful to >>> report monitor error to user. >>> >>> The patch has a drawback that on destroying a domain when waiting for >>> event from monitor we get not very convinient error message like >> >> convenient >> >>> 'EOF from monitor' from waiting API. On the other hand if qemu crashes >>> this is more useful then 'domain is not running'. The first case >>> will be addressed in another patch. >>> >>> The patch also fixes other places where we wait for event from qemu. >>> Namely handling device removal and tray ejection. Other places which >>> used virDomainObjWait (dump, migration, save) were good because of >>> async jobs which allow concurrent destroy job. >>> >>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; >>> --- >>> src/conf/domain_conf.c | 43 ------------------------------------------- >>> src/conf/domain_conf.h | 3 --- >>> src/libvirt_private.syms | 2 -- >>> src/qemu/qemu_domain.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ >>> src/qemu/qemu_domain.h | 5 ++++- >>> src/qemu/qemu_driver.c | 6 +++--- >>> src/qemu/qemu_hotplug.c | 4 ++-- >>> src/qemu/qemu_migration.c | 12 ++++++------ >>> src/qemu/qemu_process.c | 27 ++++++++++++++++++++++----- >>> 9 files changed, 82 insertions(+), 65 deletions(-) >>> >> >> This no longer git am -3 applies and based on previous patch reviews, I >> think perhaps this needs to be redone. > > I'll resend as soon as we come to agreement on series. Now I'm not > convinced to change much (except for using distinct flag to indicate error > as mentioned in 2nd patch discussion, then I don't need 3d patch). > > See comments below. > >> >> I don't believe moving/renaming virDomainObjWait is good/right, but I'm >> sure there would be other opinions on that. Yes, QEMU is currently the >> only consumer... If it were to move, it should move to virdomainobjlist >> since that's where innards of virDomainObjPtr are managed. The fact that >> we look at ->parent.lock, well, <sigh>... > > I would not move the function without reason. It gets new name qemuDomainObjWait > becase it use qemu specific data, namely monError. Today only qemu uses this generic virDomainObjWait which is generic without the need to have/use qemu specific things. Other domain consumers could use it if they chose. I'm not in favor of moving, renaming, repurposing for a very specific issue for what is a generically used function. Maybe that's just me - so you could try to get a different reviewer if you want.

> >> >> Anyway, you're attempting to special case something and perhaps you just >> need to create a qemuDomainObjWait that would call the timeout version >> of the virDomainObjWait and be able to handle whether some other error >> occurred. Wouldn't the LastError before the current wait return NULL >> (as in no error), then during the timeout period if LastError is >> something couldn't that indicate the failure you're looking for. > > I introduced qemuDomainObjWait not to put virDomainObjWait and virDomainObjWaitUntil > in the first place but to check monError. Then rather then keeping too functions > it's look more simple to use only one and branch on given timeout. > I would think if the goal was to have specific code for a specific purpose for specific functions, then introduce the qemuDomainObjWait, but have it call virDomainObjWait[Until] based on the need you have. Which in this case appears to fiddle with monError in some way. Again - that's just my opinion John

>> >> I didn't spend a lot of time thinking about alternatives and how the >> code should change, but when you mention the patch has a drawback - that >> immediately raises concern. > > But ... I addressed this issue in next patch as I wrote. > [...]