[libvirt] [PATCHv3 00/19] qemu block-commit support
Laine Stump
laine at laine.org
Thu Oct 18 20:48:37 UTC 2012
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.)
More information about the libvir-list
mailing list