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

Re: [libvirt] [PATCHv3 00/19] qemu block-commit support



On Thu, Oct 18, 2012 at 3:48 PM, Laine Stump <laine laine org> 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


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