On 05/22/2014 07:47 AM, Peter Krempa wrote: > Refresh the disk backing chains when reconnecting to a qemu process > after daemon restart. There are a few internal fields that don't get > refreshed from the XML. Until we are able to do that, let's reload all > the metadata by the backing chain crawler. Not necessarily ideal. Reading metadata from a file that is also simultaneously opened read-write by qemu might, in super-rare circumstances, hit a race where qemu is in the middle of updating metadata due to the completion of some job that was started before the libvirtd restart. Although qcow2 has a hard cap of header fitting in one cluster (typically 64k) (at least, after CVE-2014-0144, qemu.git commit 24342f2ca), it is still plausible that qemu is updating the qcow2 header via sectors (perhaps as small as 512-byte chunks) rather than by full clusters; and there are scenarios where an update touches two non-adjacent sectors (such as adjusting the backing file name), such that if a reader sees the old content of one sector but the new content of the other, then it is completely broken in interpreting the data. A slightly safer alternative might be using a QMP command to ask qemu what the backing chain currently is when we reconnect - although that may require correlation back to filenames we passed in, since in the face of fd passing, qemu may only know the name "/dev/fdset/NNN" instead of the filename the earlier libvirtd opened and passed in by fd. And that presupposes that qemu can always be trusted (we don't want to introduce a CVE where a guest that compromises qemu into returning bogus QMP results can then cause libvirt to mismanage the host filesystem). So with those points made, a metadata read race is extremely rare, and I see no point in implementing a half-way solution that requires parsing a QMP command if we are eventually going to switch over to full XML tracking anyways. Therefore, since _this_ patch is a clear improvement (even if not theoretically perfect), we shouldn't hold it up waiting for some other solution. ACK as-is. > src/qemu/qemu_process.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index a83780f..4aa9ca3 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -3213,6 +3213,11 @@ qemuProcessReconnect(void *opaque) > if (qemuTranslateDiskSourcePool(conn, obj->def->disks[i]) < 0) > goto error; > > + /* XXX we should be able to restore all data from XML in the future */ and thanks for the comment to remind us what we should improve. > + if (qemuDomainDetermineDiskChain(driver, obj, > + obj->def->disks[i], true) < 0) > + goto error; > + > dev.type = VIR_DOMAIN_DEVICE_DISK; > dev.data.disk = obj->def->disks[i]; > if (qemuAddSharedDevice(driver, &dev, obj->def->name) < 0) > -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list