Re: [PATCH 5/7] qemu: Wire up PR_MANAGER_STATUS_CHANGED event

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

 



On 07/04/2018 01:42 PM, Peter Krempa wrote:
> On Wed, Jul 04, 2018 at 12:46:53 +0200, Michal Privoznik wrote:
>> This event is emitted on the monitor if one of pr-managers lost
>> connection to its pr-helper process. What libvirt needs to do is
>> restart the pr-helper process iff it corresponds to managed
>> pr-manager.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
>> ---
>>  src/qemu/qemu_monitor.c      | 15 +++++++++++++
>>  src/qemu/qemu_monitor.h      | 11 +++++++++
>>  src/qemu/qemu_monitor_json.c | 23 +++++++++++++++++++
>>  src/qemu/qemu_process.c      | 53 ++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 102 insertions(+)
> 
> [...]
> 
> 
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index f200729cb1..94b7de76d7 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -1615,6 +1615,58 @@ qemuProcessHandleDumpCompleted(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
>>  }
>>  
>>  
>> +static int
>> +qemuProcessHandlePRManagerStatusChanged(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
>> +                                        virDomainObjPtr vm,
>> +                                        const char *prManager,
>> +                                        bool connected,
>> +                                        void *opaque ATTRIBUTE_UNUSED)
>> +{
>> +    qemuDomainObjPrivatePtr priv;
>> +    size_t i;
>> +    int ret = -1;
>> +
>> +    virObjectLock(vm);
>> +
>> +    VIR_DEBUG("pr-manager %s status changed for domain %p %s connected=%d",
>> +              prManager, vm, vm->def->name, connected);
>> +
>> +    if (connected) {
>> +        /* Connect events are boring. */
>> +        ret = 0;
>> +        goto cleanup;
>> +    }
>> +    /* Disconnect events are more interesting. */
>> +
>> +    for (i = 0; i < vm->def->ndisks; i++) {
>> +        const char *mgralias;
>> +
>> +        mgralias = virStorageSourceChainGetManagedPRAlias(vm->def->disks[i]->src);
>> +
>> +        if (STREQ_NULLABLE(prManager, mgralias))
>> +            break;
> 
> I'm not a fan of this. We always know which is the managed alias

Do we? I thought the whole point of storing it in source private data
was to make it independent of qemuDomainGetManagedPRAlias().
Imagine I started a domain with pr-manager.alias = pr-helper0. Then
restarted libvirtd to upgrade it. This new version has, however,
pr-helper1 as default alias (because reasons). Then all of a sudden I
receive this event with alias pr-helper0 (because that is how I started
the domain). I can't do plain:

  STREQ(prManager, qemuDomainGetManagedPRAlias())

can I?

> and we
> also know if it is supposed to be running.
> 
> The hotplug code already does not inspect disks to do this since there
> is only one instance.

Sure. But hotplug code does not have to care about old instances.

> 
>> +    }
>> +
>> +    if (i == vm->def->ndisks) {
>> +        VIR_DEBUG("pr-manager %s not managed, ignoring event",
>> +                  prManager);
>> +        ret = 0;
>> +        goto cleanup;
>> +    }
>> +
>> +    priv = vm->privateData;
>> +    priv->prDaemonRunning = false;
>> +
>> +    if (qemuProcessStartManagedPRDaemon(vm) < 0)
> 
> This has a timeout built in. Thus executing this from the event loop
> will make the whole libvirtd get stuck until it starts. This should not
> be in the event loop.

Ah good point. I'll rewrite this to run it in the qemu event process
thread (or what do we call it).

> 
> Also does every disconnect equal to the daemon crashing/stopping?

It should. If qemu sends us bogus events I'm not sure there's much we
can do about it.

Michal

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

  Powered by Linux