Re: [libvirt] [PATCH v9 05/10] backup: Parse and output backup XML

On Mon, Jul 08, 2019 at 11:55:48 -0500, Eric Blake wrote: [...] [...] [...] [...] This leaks disk->store [...] virStorageSource MUST be allocated using virStorageSourceNew since it's an virObject ... Why is this qemuism validated in the main parser? ... > + VIR_FREE(type); > + if (ret < 0) > + virDomainBackupDiskDefClear(def); > + return ret; [...] private data? > + } > + > + if (virDomainDiskSourceFormat(buf, disk->store, push ? "target" : "scratch", > + 0, false, 0, NULL) < 0) > + goto cleanup; > + virBufferAdjustIndent(buf, -2); > + virBufferAddLit(buf, "</disk>\n"); > + > + ret = 0; > + > + cleanup: > + virBufferFreeAndReset(&attrBuf); > + virBufferFreeAndReset(&childBuf); > + return ret; > + [...] Please note in the comment that disk->store is used to switch behaviour when all_disks is true. virStorageSourceNew. I already commented once that this should not be abused. > + } else { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("refusing to generate file name for disk '%s'"), > + disk->name); > + goto cleanup; > + } > + } > + ret = 0; > + cleanup: > + return ret; > + > +/* Align def->disks to domain. Sort the list of def->disks, > + * generating storage names using suffix as needed. Convert paths to > + * disk targets for uniformity. Issue an error and return -1 if any > + * def->disks[n]->name appears more than once or does not map to > + * dom->disks. */ > +int > +virDomainBackupAlignDisks(virDomainBackupDefPtr def, virDomainDefPtr dom, > + const char *suffix) > +{ > + int ret = -1; > + virBitmapPtr map = NULL; > + size_t i; > + int ndisks; > + bool alloc_all = false; > + > + if (def->ndisks > dom->ndisks) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("too many disk backup requests for domain")); > + goto cleanup; > + } > + > + /* Unlikely to have a guest without disks but technically possible. */ > + if (!dom->ndisks) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("domain must have at least one disk to perform " > + "backups")); > + goto cleanup; > + } > + > + if (!(map = virBitmapNew(dom->ndisks))) > + goto cleanup; > + > + /* Double check requested disks. */ > + for (i = 0; i < def->ndisks; i++) { > + virDomainBackupDiskDefPtr disk = &def->disks[i]; > + int idx = virDomainDiskIndexByName(dom, disk->name, false); > + > + if (idx < 0) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("no disk named '%s'"), disk->name); > + goto cleanup; > + } > + > + if (virBitmapIsBitSet(map, idx)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("disk '%s' specified twice"), > + disk->name); > + goto cleanup; > + } > + ignore_value(virBitmapSetBit(map, idx)); > + disk->idx = idx; > + > + if (STRNEQ(disk->name, dom->disks[idx]->dst)) { > + VIR_FREE(disk->name); > + if (VIR_STRDUP(disk->name, dom->disks[idx]->dst) < 0) > + goto cleanup; > + } > + if (disk->store && !disk->store->path) { > + virStorageSourceClear(disk->store); > + disk->store = NULL; This will leak disk->store.