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

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



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 redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


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