[libvirt] [PATCH] qemu: Check for ejected media during startup and migration

[Eric Blake]

On 09/13/2011 10:14 AM, Michal Privoznik wrote:
> If the daemon is restarted so we reconnect to monitor, cdrom media
> can be ejected. In that case we don't want to show it in domain xml,
> or require it on migration destination.
>
> To check for disk status use 'info block' monitor command.
> ---
> NB, the 'info block' is currently not updated yet. The qemu patches
> that extend the monitor command are in a queue (that will be merged
> quickly):
>      http://repo.or.cz/w/qemu/kevin.git
> head for-anthony
> The commit that introduce requested feature:
>      http://repo.or.cz/w/qemu/kevin.git/commitdiff/e4def80b36231e161b91fa984cd0d73b45668f00

Looks like this is in upstream qemu.git now, so that hurdle is out of 
the way; guess I'd better give my formal review :)

My original complaint was that this was polling-based, but you've 
convinced me that: 1) polling is necessary for libvirtd restart to learn 
the correct state (since events while libvirtd is down are lost), and 2) 
this doesn't touch user-visible XML, so the polling is limited to a few 
points in time, mainly where we were already talking to the monitor anyway.

If, in the future, qemu does add event tracking, we can then expose 
user-visible XML to expose this state to the user (whereas this patch 
just uses the information internally); at that point, we'd want to only 
expose XML if it can avoid polling (since we can't control how 
frequently dumpxml is called, and don't want to involve the monitor on 
every user-triggered dumpxml).  But that's stuff for a later day, which 
does not affect this patch.

> +++ b/src/qemu/qemu_driver.c
> @@ -8236,6 +8236,13 @@ qemuDomainMigrateBegin3(virDomainPtr domain,
>           goto endjob;
>       }
>
> +    /* Check if there is and ejected media.

s/and/any/

> +int
> +qemuDomainCheckEjectableMedia(struct qemud_driver *driver,
> +                             virDomainObjPtr vm)
> +{
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    int ret = -1;
> +    int i;
> +
> +    for (i = 0; i<  vm->def->ndisks; i++) {
> +        virDomainDiskDefPtr disk = vm->def->disks[i];
> +        struct qemuDomainDiskInfo info;
> +
> +        memset(&info, 0, sizeof(info));
> +
> +        qemuDomainObjEnterMonitor(driver, vm);

This does a monitor command for every disk, even for non-floppy 
non-cdrom disks, where we already know the answer (if the disk is not 
ejectable, then it can't be in the ejected state).  Add a check before 
the memset():

  if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK)
     continue;

> +            while (*p) {
> +                if (STRPREFIX(p, "removable=")) {
> +                    p += strlen("removable=");
> +                    if (virStrToLong_i(p,&dummy, 10,&tmp) == -1)
> +                        VIR_DEBUG("error reading removable: %s", p);
> +                    else
> +                        info->removable = p ? true : false;

I tend to write this as "!!p" or "p != NULL", rather than the longer "p 
? true : false", but that's not a show-stopper.

I'm comfortable giving:

ACK with nits fixed.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org