On Thu, Oct 18, 2012 at 3:48 PM, Laine Stump <laine@xxxxxxxxx> wrote: > 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.) > I read through the diff and I concur to extend my ACKs. -- Doug Goldstein -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list