On 10/17/2012 06:30 PM, Eric Blake wrote: > This v3 posting resolves all the comments I had from Doug, Laine, > and myself, and has passed my testing with SELinux enabled. > > v2 was here: > https://www.redhat.com/archives/libvir-list/2012-October/msg00633.html > See below for interdiff, and individual patches for more notes > about changes > > Also available at: > http://repo.or.cz/w/libvirt/ericb.git/shortlog/refs/heads/blockjob > git fetch git://repo.or.cz/libvirt/ericb.git blockjob > > Eric Blake (19): > storage: list more file types > storage: treat 'aio' like 'raw' at parse time > storage: match RNG to supported driver types > storage: use enum for default driver type > storage: use enum for disk driver type > storage: use enum for snapshot driver type > storage: don't probe non-files > storage: get entire metadata chain in one call > storage: don't require caller to pre-allocate metadata struct > storage: remember relative names in backing chain > storage: make it easier to find file within chain > storage: cache backing chain while qemu domain is live > storage: use cache to walk backing chain > blockjob: remove unused parameters after previous patch > blockjob: manage qemu block-commit monitor command > blockjob: wire up online qemu block-commit > blockjob: implement shallow commit flag in qemu > blockjob: refactor qemu disk chain permission grants > blockjob: properly label disks for qemu block-commit > > docs/schemas/domaincommon.rng | 27 +- > docs/schemas/domainsnapshot.rng | 2 +- > src/conf/capabilities.h | 4 +- > src/conf/domain_conf.c | 165 +++------ > src/conf/domain_conf.h | 8 +- > src/conf/snapshot_conf.c | 23 +- > src/conf/snapshot_conf.h | 2 +- > src/conf/storage_conf.c | 15 +- > src/libvirt.c | 2 - > src/libvirt_private.syms | 1 + > src/libxl/libxl_conf.c | 42 ++- > src/libxl/libxl_driver.c | 6 +- > src/qemu/qemu_capabilities.c | 3 + > src/qemu/qemu_capabilities.h | 1 + > src/qemu/qemu_cgroup.c | 14 +- > src/qemu/qemu_cgroup.h | 6 +- > src/qemu/qemu_command.c | 18 +- > src/qemu/qemu_domain.c | 33 +- > src/qemu/qemu_domain.h | 3 + > src/qemu/qemu_driver.c | 371 ++++++++++++++------- > src/qemu/qemu_hotplug.c | 13 +- > src/qemu/qemu_monitor.c | 30 ++ > src/qemu/qemu_monitor.h | 7 + > src/qemu/qemu_monitor_json.c | 34 ++ > src/qemu/qemu_monitor_json.h | 7 + > src/qemu/qemu_process.c | 11 + > src/security/security_dac.c | 7 - > src/security/security_selinux.c | 11 - > src/security/virt-aa-helper.c | 20 +- > src/storage/storage_backend_fs.c | 15 +- > src/util/storage_file.c | 282 ++++++++++++---- > src/util/storage_file.h | 43 ++- > src/vbox/vbox_tmpl.c | 6 +- > src/xenxs/xen_sxpr.c | 26 +- > src/xenxs/xen_xm.c | 28 +- > tests/sexpr2xmldata/sexpr2xml-curmem.xml | 2 +- > .../sexpr2xml-disk-block-shareable.xml | 2 +- > .../sexpr2xml-disk-drv-blktap-raw.xml | 2 +- > .../sexpr2xml-disk-drv-blktap2-raw.xml | 2 +- > 39 files changed, 859 insertions(+), 435 deletions(-) > > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 81cb3aa..afa4cfe 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -971,7 +971,7 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def) > VIR_FREE(def->src); > VIR_FREE(def->dst); > VIR_FREE(def->driverName); > - virStorageFileFreeMetadata(def->chain); > + virStorageFileFreeMetadata(def->backingChain); > VIR_FREE(def->mirror); > VIR_FREE(def->auth.username); > VIR_FREE(def->wwn); > @@ -3723,8 +3723,12 @@ virDomainDiskDefParseXML(virCapsPtr caps, > xmlStrEqual(cur->name, BAD_CAST "driver")) { > driverName = virXMLPropString(cur, "name"); > driverType = virXMLPropString(cur, "type"); > - if (STREQ_NULLABLE(driverType, "aio")) > - memcpy(driverType, "raw", strlen("raw")); > + if (STREQ_NULLABLE(driverType, "aio")) { > + /* In-place conversion to "raw", for Xen back-compat */ > + driverType[0] = 'r'; > + driverType[1] = 'a'; > + driverType[2] = 'w'; > + } > cachetag = virXMLPropString(cur, "cache"); > error_policy = virXMLPropString(cur, "error_policy"); > rerror_policy = virXMLPropString(cur, "rerror_policy"); > @@ -4185,7 +4189,8 @@ virDomainDiskDefParseXML(virCapsPtr caps, > driverType); > goto error; > } > - } else { > + } else if (def->type == VIR_DOMAIN_DISK_TYPE_FILE || > + def->type == VIR_DOMAIN_DISK_TYPE_BLOCK) { > def->format = caps->defaultDiskDriverType; > } > > @@ -14763,10 +14768,10 @@ done: > > > /* Call iter(disk, name, depth, opaque) for each element of disk and > - its backing chain in the pre-populated disk->chain. > - ignoreOpenFailure determines whether to warn about a chain that > - mentions a backing file without also having metadata on that > - file. */ > + * its backing chain in the pre-populated disk->backingChain. > + * ignoreOpenFailure determines whether to warn about a chain that > + * mentions a backing file without also having metadata on that > + * file. */ > int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, > bool ignoreOpenFailure, > virDomainDiskDefPathIterator iter, > @@ -14782,7 +14787,7 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, > if (iter(disk, disk->src, 0, opaque) < 0) > goto cleanup; > > - tmp = disk->chain; > + tmp = disk->backingChain; > while (tmp && tmp->backingStoreIsFile) { > if (!ignoreOpenFailure && !tmp->backingMeta) { > virReportError(VIR_ERR_INTERNAL_ERROR, > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index 9c3abec..10ef841 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -569,7 +569,7 @@ struct _virDomainDiskDef { > } auth; > char *driverName; > int format; /* enum virStorageFileFormat */ > - virStorageFileMetadataPtr chain; > + virStorageFileMetadataPtr backingChain; > > char *mirror; > int mirrorFormat; /* enum virStorageFileFormat */ > diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c > index 428befd..db371a0 100644 > --- a/src/qemu/qemu_cgroup.c > +++ b/src/qemu/qemu_cgroup.c > @@ -87,8 +87,7 @@ qemuSetupDiskPathAllow(virDomainDiskDefPtr disk, > } > > > -int qemuSetupDiskCgroup(struct qemud_driver *driver ATTRIBUTE_UNUSED, > - virDomainObjPtr vm, > +int qemuSetupDiskCgroup(virDomainObjPtr vm, > virCgroupPtr cgroup, > virDomainDiskDefPtr disk) > { > @@ -127,8 +126,7 @@ qemuTeardownDiskPathDeny(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, > } > > > -int qemuTeardownDiskCgroup(struct qemud_driver *driver ATTRIBUTE_UNUSED, > - virDomainObjPtr vm, > +int qemuTeardownDiskCgroup(virDomainObjPtr vm, > virCgroupPtr cgroup, > virDomainDiskDefPtr disk) > { > @@ -230,7 +228,7 @@ int qemuSetupCgroup(struct qemud_driver *driver, > for (i = 0; i < vm->def->ndisks ; i++) { > if (qemuDomainDetermineDiskChain(driver, vm->def->disks[i], > false) < 0 || > - qemuSetupDiskCgroup(driver, vm, cgroup, vm->def->disks[i]) < 0) > + qemuSetupDiskCgroup(vm, cgroup, vm->def->disks[i]) < 0) > goto cleanup; > } > > diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h > index 362080a..c552162 100644 > --- a/src/qemu/qemu_cgroup.h > +++ b/src/qemu/qemu_cgroup.h > @@ -36,12 +36,10 @@ typedef struct _qemuCgroupData qemuCgroupData; > > bool qemuCgroupControllerActive(struct qemud_driver *driver, > int controller); > -int qemuSetupDiskCgroup(struct qemud_driver *driver, > - virDomainObjPtr vm, > +int qemuSetupDiskCgroup(virDomainObjPtr vm, > virCgroupPtr cgroup, > virDomainDiskDefPtr disk); > -int qemuTeardownDiskCgroup(struct qemud_driver *driver, > - virDomainObjPtr vm, > +int qemuTeardownDiskCgroup(virDomainObjPtr vm, > virCgroupPtr cgroup, > virDomainDiskDefPtr disk); > int qemuSetupHostUsbDeviceCgroup(usbDevice *dev, > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index f071769..4196caf 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -2129,7 +2129,7 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, > disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)) { > if (disk->type == VIR_DOMAIN_DISK_TYPE_DIR) { > /* QEMU only supports magic FAT format for now */ > - if (disk->format && disk->format != VIR_STORAGE_FILE_FAT) { > + if (disk->format > 0 && disk->format != VIR_STORAGE_FILE_FAT) { > virReportError(VIR_ERR_INTERNAL_ERROR, > _("unsupported disk driver type for '%s'"), > virStorageFileFormatTypeToString(disk->format)); > @@ -5210,7 +5210,7 @@ qemuBuildCommandLine(virConnectPtr conn, > > if (disk->type == VIR_DOMAIN_DISK_TYPE_DIR) { > /* QEMU only supports magic FAT format for now */ > - if (disk->format && disk->format != VIR_STORAGE_FILE_FAT) { > + if (disk->format > 0 && disk->format != VIR_STORAGE_FILE_FAT) { > virReportError(VIR_ERR_INTERNAL_ERROR, > _("unsupported disk driver type for '%s'"), > virStorageFileFormatTypeToString(disk->format)); > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index 9675454..45f3a5e 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -2016,30 +2016,23 @@ qemuDomainDetermineDiskChain(struct qemud_driver *driver, > virDomainDiskDefPtr disk, > bool force) > { > - int format; > + bool probe = driver->allowDiskFormatProbing; > > if (!disk->src) > return 0; > > - if (disk->chain) { > + if (disk->backingChain) { > if (force) { > - virStorageFileFreeMetadata(disk->chain); > - disk->chain = NULL; > + virStorageFileFreeMetadata(disk->backingChain); > + disk->backingChain = NULL; > } else { > return 0; > } > } > - if (disk->format > 0) > - format = disk->format; > - else if (driver->allowDiskFormatProbing) > - format = VIR_STORAGE_FILE_AUTO; > - else > - format = VIR_STORAGE_FILE_RAW; > - > - disk->chain = virStorageFileGetMetadata(disk->src, format, > - driver->user, driver->group, > - driver->allowDiskFormatProbing); > - if (!disk->chain && !force) > + disk->backingChain = virStorageFileGetMetadata(disk->src, disk->format, > + driver->user, driver->group, > + probe); > + if (!disk->backingChain) > return -1; > return 0; > } > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 7a47cf7..3829a89 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -5826,7 +5826,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, > vm->def->name); > goto end; > } > - if (qemuSetupDiskCgroup(driver, vm, cgroup, disk) < 0) > + if (qemuSetupDiskCgroup(vm, cgroup, disk) < 0) > goto end; > } > switch (disk->device) { > @@ -5862,7 +5862,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, > } > > if (ret != 0 && cgroup) { > - if (qemuTeardownDiskCgroup(driver, vm, cgroup, disk) < 0) > + if (qemuTeardownDiskCgroup(vm, cgroup, disk) < 0) > VIR_WARN("Failed to teardown cgroup for disk path %s", > NULLSTR(disk->src)); > } > @@ -6058,7 +6058,7 @@ qemuDomainChangeDiskMediaLive(virDomainObjPtr vm, > vm->def->name); > goto end; > } > - if (qemuSetupDiskCgroup(driver, vm, cgroup, disk) < 0) > + if (qemuSetupDiskCgroup(vm, cgroup, disk) < 0) > goto end; > } > > @@ -6077,7 +6077,7 @@ qemuDomainChangeDiskMediaLive(virDomainObjPtr vm, > } > > if (ret != 0 && cgroup) { > - if (qemuTeardownDiskCgroup(driver, vm, cgroup, disk) < 0) > + if (qemuTeardownDiskCgroup(vm, cgroup, disk) < 0) > VIR_WARN("Failed to teardown cgroup for disk path %s", > NULLSTR(disk->src)); > } > @@ -10462,7 +10462,8 @@ typedef enum { > /* Several operations end up adding or removing a single element of a > * disk backing file chain; this helper function ensures that the lock > * manager, cgroup device controller, and security manager labelling > - * are all aware of each new file before it is added to a chain. */ > + * are all aware of each new file before it is added to a chain, and > + * can revoke access to a file no longer needed in a chain. */ > static int > qemuDomainPrepareDiskChainElement(struct qemud_driver *driver, > virDomainObjPtr vm, > @@ -10476,26 +10477,26 @@ qemuDomainPrepareDiskChainElement(struct qemud_driver *driver, > * temporarily modify the disk in place. */ > char *origsrc = disk->src; > int origformat = disk->format; > - virStorageFileMetadataPtr origchain = disk->chain; > + virStorageFileMetadataPtr origchain = disk->backingChain; > bool origreadonly = disk->readonly; > int ret = -1; > > disk->src = (char *) file; /* casting away const is safe here */ > disk->format = VIR_STORAGE_FILE_RAW; > - disk->chain = NULL; > + disk->backingChain = NULL; > disk->readonly = mode == VIR_DISK_CHAIN_READ_ONLY; > > if (mode == VIR_DISK_CHAIN_NO_ACCESS) { > if (virSecurityManagerRestoreImageLabel(driver->securityManager, > vm->def, disk) < 0) > VIR_WARN("Unable to restore security label on %s", disk->src); > - if (cgroup && qemuTeardownDiskCgroup(driver, vm, cgroup, disk) < 0) > + if (cgroup && qemuTeardownDiskCgroup(vm, cgroup, disk) < 0) > VIR_WARN("Failed to teardown cgroup for disk path %s", disk->src); > if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) > VIR_WARN("Unable to release lock on %s", disk->src); > } else if (virDomainLockDiskAttach(driver->lockManager, driver->uri, > vm, disk) < 0 || > - (cgroup && qemuSetupDiskCgroup(driver, vm, cgroup, disk) < 0) || > + (cgroup && qemuSetupDiskCgroup(vm, cgroup, disk) < 0) || > virSecurityManagerSetImageLabel(driver->securityManager, > vm->def, disk) < 0) { > goto cleanup; > @@ -10506,7 +10507,7 @@ qemuDomainPrepareDiskChainElement(struct qemud_driver *driver, > cleanup: > disk->src = origsrc; > disk->format = origformat; > - disk->chain = origchain; > + disk->backingChain = origchain; > disk->readonly = origreadonly; > return ret; > } > @@ -10802,7 +10803,6 @@ cleanup: > return ret; > } > > - > /* The domain is expected to hold monitor lock. */ > static int > qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, > @@ -10848,14 +10848,14 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, > VIR_FORCE_CLOSE(fd); > } > > - /* XXX Here, we know we are about to alter disk->chain if > + /* XXX Here, we know we are about to alter disk->backingChain if > * successful, so we nuke the existing chain so that future > * commands will recompute it. Better would be storing the chain > * ourselves rather than reprobing, but this requires modifying > * domain_conf and our XML to fully track the chain across > * libvirtd restarts. */ > - virStorageFileFreeMetadata(disk->chain); > - disk->chain = NULL; > + virStorageFileFreeMetadata(disk->backingChain); > + disk->backingChain = NULL; > > if (qemuDomainPrepareDiskChainElement(driver, vm, cgroup, disk, source, > VIR_DISK_CHAIN_READ_WRITE) < 0) { > @@ -12737,8 +12737,9 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, > > if (!top) { > top_canon = disk->src; > - top_meta = disk->chain; > - } else if (!(top_canon = virStorageFileChainLookup(disk->chain, disk->src, > + top_meta = disk->backingChain; > + } else if (!(top_canon = virStorageFileChainLookup(disk->backingChain, > + disk->src, > top, &top_meta, > &top_parent))) { > virReportError(VIR_ERR_INVALID_ARG, > @@ -12762,6 +12763,9 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, > base ? base : "(default)", top_canon, path); > goto endjob; > } > + /* Note that this code exploits the fact that > + * virStorageFileChainLookup guarantees a simple pointer > + * comparison will work, rather than needing full-blown STREQ. */ > if ((flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW) && > base_canon != top_meta->backingStore) { > virReportError(VIR_ERR_INVALID_ARG, > @@ -14165,7 +14169,7 @@ static virDriver qemuDriver = { > .domainBlockJobSetSpeed = qemuDomainBlockJobSetSpeed, /* 0.9.4 */ > .domainBlockPull = qemuDomainBlockPull, /* 0.9.4 */ > .domainBlockRebase = qemuDomainBlockRebase, /* 0.9.10 */ > - .domainBlockCommit = qemuDomainBlockCommit, /* 0.10.3 */ > + .domainBlockCommit = qemuDomainBlockCommit, /* 1.0.0 */ > .isAlive = qemuIsAlive, /* 0.9.8 */ > .nodeSuspendForDuration = nodeSuspendForDuration, /* 0.9.8 */ > .domainSetBlockIoTune = qemuDomainSetBlockIoTune, /* 0.9.8 */ > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index ca441f2..7381921 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c > @@ -2006,7 +2006,7 @@ int qemuDomainDetachPciDiskDevice(struct qemud_driver *driver, > VIR_WARN("Unable to restore security label on %s", dev->data.disk->src); > > if (cgroup != NULL) { > - if (qemuTeardownDiskCgroup(driver, vm, cgroup, dev->data.disk) < 0) > + if (qemuTeardownDiskCgroup(vm, cgroup, dev->data.disk) < 0) > VIR_WARN("Failed to teardown cgroup for disk path %s", > NULLSTR(dev->data.disk->src)); > } > @@ -2089,7 +2089,7 @@ int qemuDomainDetachDiskDevice(struct qemud_driver *driver, > VIR_WARN("Unable to restore security label on %s", dev->data.disk->src); > > if (cgroup != NULL) { > - if (qemuTeardownDiskCgroup(driver, vm, cgroup, dev->data.disk) < 0) > + if (qemuTeardownDiskCgroup(vm, cgroup, dev->data.disk) < 0) > VIR_WARN("Failed to teardown cgroup for disk path %s", > NULLSTR(dev->data.disk->src)); > } > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c > index d0ecd1e..d1ce9cc 100644 > --- a/src/qemu/qemu_monitor_json.c > +++ b/src/qemu/qemu_monitor_json.c > @@ -3284,7 +3284,7 @@ cleanup: > int > qemuMonitorJSONBlockCommit(qemuMonitorPtr mon, const char *device, > const char *top, const char *base, > - unsigned long speed) > + unsigned long long speed) > { > int ret = -1; > virJSONValuePtr cmd; > diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h > index 71bc6aa..61127a7 100644 > --- a/src/qemu/qemu_monitor_json.h > +++ b/src/qemu/qemu_monitor_json.h > @@ -239,7 +239,7 @@ int qemuMonitorJSONBlockCommit(qemuMonitorPtr mon, > const char *device, > const char *top, > const char *base, > - unsigned long bandwidth) > + unsigned long long bandwidth) > ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); > > int qemuMonitorJSONArbitraryCommand(qemuMonitorPtr mon, > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index 1eb93a6..3a087e2 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -4121,18 +4121,6 @@ void qemuProcessStop(struct qemud_driver *driver, > networkReleaseActualDevice(net); > } > > - /* XXX For now, disk chains should only be cached while qemu is > - * running. Since we don't track the chain in XML, a user is free > - * to update the chain while the domain is offline, and then when > - * they next boot the domain we should re-read the chain from the > - * files at that point in time. Only when we track the chain in > - * XML can we forbid the user from altering the chain of an > - * offline domain. */ > - for (i = 0; i < def->ndisks; i++) { > - virStorageFileFreeMetadata(def->disks[i]->chain); > - def->disks[i]->chain = NULL; > - } > - > retry: > if ((ret = qemuRemoveCgroup(driver, vm, 0)) < 0) { > if (ret == -EBUSY && (retries++ < 5)) { > diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c > index 729c0d1..263fc92 100644 > --- a/src/security/virt-aa-helper.c > +++ b/src/security/virt-aa-helper.c > @@ -919,21 +919,13 @@ get_files(vahControl * ctl) > } > > for (i = 0; i < ctl->def->ndisks; i++) { > - int ret; > - int format; > virDomainDiskDefPtr disk = ctl->def->disks[i]; > > - if (disk->format > 0) > - format = disk->format; > - else if (ctl->allowDiskFormatProbing) > - format = VIR_STORAGE_FILE_AUTO; > - else > - format = VIR_STORAGE_FILE_RAW; > - > /* XXX - if we knew the qemu user:group here we could send it in > * so that the open could be re-tried as that user:group. > */ > - disk->chain = virStorageFileGetMetadata(disk->src, format, -1, -1, > + disk->chain = virStorageFileGetMetadata(disk->src, disk->format, > + -1, -1, > ctl->allowDiskFormatProbing, > NULL); > > @@ -942,8 +934,7 @@ get_files(vahControl * ctl) > * be passing ignoreOpenFailure = false and handle open errors more > * careful than just ignoring them. > */ > - ret = virDomainDiskDefForeachPath(disk, true, add_file_path, &buf); > - if (ret != 0) > + if (virDomainDiskDefForeachPath(disk, true, add_file_path, &buf) < 0) > goto clean; > } > > diff --git a/src/util/storage_file.c b/src/util/storage_file.c > index 218891e..882df6e 100644 > --- a/src/util/storage_file.c > +++ b/src/util/storage_file.c > @@ -271,7 +271,8 @@ qcow2GetBackingStoreFormat(int *format, > break; > *format = virStorageFileFormatTypeFromString( > ((const char *)buf)+offset); > - break; > + if (*format <= VIR_STORAGE_FILE_NONE) > + return -1; > } > > offset += len; > @@ -353,12 +354,10 @@ qcowXGetBackingStore(char **res, > * between the end of the header (QCOW2_HDR_TOTAL_SIZE) > * and the start of the backingStoreName (offset) > */ > - if (isQCow2 && format) { > + if (isQCow2 && format && > qcow2GetBackingStoreFormat(format, buf, buf_size, QCOW2_HDR_TOTAL_SIZE, > - offset); > - if (*format <= VIR_STORAGE_FILE_NONE) > - return BACKING_STORE_INVALID; > - } > + offset) < 0) > + return BACKING_STORE_INVALID; > > return BACKING_STORE_OK; > } > @@ -517,7 +516,7 @@ qedGetBackingStore(char **res, > (*res)[size] = '\0'; > > if (flags & QED_F_BACKING_FORMAT_NO_PROBE) > - *format = virStorageFileFormatTypeFromString("raw"); > + *format = VIR_STORAGE_FILE_RAW; > else > *format = VIR_STORAGE_FILE_AUTO_SAFE; > > @@ -954,7 +953,7 @@ virStorageFileGetMetadataRecurse(const char *path, int format, > ret = virStorageFileGetMetadataFromFD(path, fd, format); > > if (VIR_CLOSE(fd) < 0) > - virReportSystemError(errno, _("could not close file %s"), path); > + VIR_WARN("could not close file %s", path); > > if (ret && ret->backingStoreIsFile) { > if (ret->backingStoreFormat == VIR_STORAGE_FILE_AUTO && !allow_probe) > @@ -1004,6 +1003,9 @@ virStorageFileGetMetadata(const char *path, int format, > > if (!cycle) > return NULL; > + > + if (format <= VIR_STORAGE_FILE_NONE) > + format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW; > ret = virStorageFileGetMetadataRecurse(path, format, uid, gid, > allow_probe, cycle); > virHashFree(cycle); > @@ -1261,13 +1263,14 @@ const char *virStorageFileGetSCSIKey(const char *path) > } > #endif > > -/* Given a CHAIN that starts at the named file START, return the > - * canonical name for the backing file NAME within that chain, or pass > - * NULL to find the base of the chain. If *META is not NULL, set it > +/* Given a CHAIN that starts at the named file START, return a string > + * pointing to either START or within CHAIN that gives the preferred > + * name for the backing file NAME within that chain. Pass NULL for > + * NAME to find the base of the chain. If META is not NULL, set *META > * to the point in the chain that describes NAME (or to NULL if the > - * backing element is not a file). If *PARENT is not NULL, set it to > - * the canonical name of the parent (or to NULL if NAME matches > - * START). The results point within CHAIN, and must not be > + * backing element is not a file). If PARENT is not NULL, set *PARENT > + * to the preferred name of the parent (or to NULL if NAME matches > + * START). Since the results point within CHAIN, they must not be > * independently freed. */ > const char * > virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *start, > @@ -1301,12 +1304,12 @@ virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *start, > STREQ(name, owner->backingStore)) { > break; > } else if (owner->backingStoreIsFile) { > - char *abs = absolutePathFromBaseFile(*parent, name); > - if (abs && STREQ(abs, owner->backingStore)) { > - VIR_FREE(abs); > + char *absName = absolutePathFromBaseFile(*parent, name); > + if (absName && STREQ(absName, owner->backingStore)) { > + VIR_FREE(absName); > break; > } > - VIR_FREE(abs); > + VIR_FREE(absName); > } > *parent = owner->backingStore; > owner = owner->backingMeta; > > I *think* I recognized everything in the interdiff from my comments, or else they made sense. So ACK to the interdiffs, and I guess if Doug has already ACKed the v2 of PATCH 01-07, then this should extend those ACKs up to v3 (does that make sense? At any rate, you have my explicit ACKs on path 08-19, and on the v2-v3 differences to patch 01-08.) -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list