Re: [libvirt] [PATCH v4 12/20] wip: backup: Parse and output backup XML

On 2/6/19 2:18 PM, Eric Blake wrote: I think this should be it's own module src/conf/backup_conf.{c,h} Like checkpoint, the "struct _vir*" probably should go with in the .c files and accessors created. Similar to previous patch. Could also have the VIR_DEFINE_AUTOPTR_FUNC(virDomainBackupDef, virDomainBackupDefFree); Initialize idx and state? Similar to previous w/ 2 blank lines. True... You'll need some what to Free only what you've allocated or if there's a generic Server*Free function to use it. > + for (i = 0; i < def->ndisks; i++) > + virDomainBackupDiskDefClear(&def->disks[i]); > + VIR_FREE(def->disks); > + VIR_FREE(def); > + > +static int > +virDomainBackupDiskDefParseXML(xmlNodePtr node, > + xmlXPathContextPtr ctxt, > + virDomainBackupDiskDefPtr def, > + bool push, bool internal, > + virDomainXMLOptionPtr xmlopt) > +{ > + int ret = -1; > + // char *backup = NULL; /* backup="yes|no"? */ > + char *type = NULL; > + char *driver = NULL; > + xmlNodePtr cur; > + xmlNodePtr saved = ctxt->node; VIR_AUTOFREE(char *) type = NULL; VIR_AUTOFREE(char *) driver = NULL; Rather than this you could have a @store which would stolen at the end before cleanup if everything has passed through parsing. I'm in the processing of adding a VIR_AUTOPTR(virStorageSource) too. Still need to decide... Is this another way of using FLAGS to determine the parse mode? IOW, is this only needed when parsing the status/running XML. VIR_FREE's won't be necessary w/ autofree stuff. Unnecessary since the caller will do this on error with virDomainBackupDefFree > + return ret; > + > +static virDomainBackupDefPtr > +virDomainBackupDefParse(xmlXPathContextPtr ctxt, > + virDomainXMLOptionPtr xmlopt, > + unsigned int flags) > +{ > + virDomainBackupDefPtr def = NULL; > + virDomainBackupDefPtr ret = NULL; > + xmlNodePtr *nodes = NULL; > + xmlNodePtr node = NULL; > + char *mode = NULL; > + bool push; > + size_t i; > + int n; VIR_AUTOPTR(virDomainBackupDef) def = NULL; VIR_AUTOFREE(char *) mode = NULL; VIR_AUTOFREE(xmlNodePtr *) nodes = NULL; You can use VIR_AUTOFREE(char *) tmp too... Ahh... so this is where it's referenced... I recall this from RNG... If transport == FOO is ever added and not supported, then you're in trouble... Go with if (def->server->transport != VIR_STORAGE_NET_HOST_TRANS_...) {TCP|UNIX} and use the virStorageHostNetTransportTypeToString to translate what is not supported... Autofree removes the need for cleanup here, so we could return NULL directly on failure paths. > + > + return ret; > + > +virDomainBackupDefPtr > +virDomainBackupDefParseString(const char *xmlStr, > + virDomainXMLOptionPtr xmlopt, > + unsigned int flags) > +{ > + virDomainBackupDefPtr ret = NULL; > + xmlDocPtr xml; > + int keepBlanksDefault = xmlKeepBlanksDefault(0); > + > + if ((xml = virXMLParse(NULL, xmlStr, _("(domain_backup)")))) { > + xmlKeepBlanksDefault(keepBlanksDefault); > + ret = virDomainBackupDefParseNode(xml, xmlDocGetRootElement(xml), > + xmlopt, flags); > + xmlFreeDoc(xml); > + } > + xmlKeepBlanksDefault(keepBlanksDefault); > + > + return ret; > + > +virDomainBackupDefPtr > +virDomainBackupDefParseNode(xmlDocPtr xml, > + xmlNodePtr root, > + virDomainXMLOptionPtr xmlopt, > + unsigned int flags) > +{ > + xmlXPathContextPtr ctxt = NULL; > + virDomainBackupDefPtr def = NULL; > + > + if (!virXMLNodeNameEqual(root, "domainbackup")) { > + virReportError(VIR_ERR_XML_ERROR, "%s", _("domainbackup")); > + goto cleanup; return NULL; return NULL; > + } > + > + ctxt->node = root; > + def = virDomainBackupDefParse(ctxt, xmlopt, flags); > + cleanup: > + xmlXPathFreeContext(ctxt); > + return def; > + > +static int > +virDomainBackupDiskDefFormat(virBufferPtr buf, > + virDomainBackupDiskDefPtr disk, > + bool push, bool internal) one arg per line Should @internal make use of flags instead? ... I assume this is the <backup> noted earlier. Do we really want to store them in two places? I think it's been hard enough with just one. Hazards of a design where the checkpoint and/or backup are not subelements of the disk I suppose. > + if (internal) { > + virBufferEscapeString(buf, "<node detected='%s'", > + disk->store->detected ? "1" : "0"); > + virBufferEscapeString(buf, ">%s</node>\n", disk->store->nodeformat); > + } > + > + virBufferSetChildIndent(&childBuf, buf); > + if (virDomainStorageSourceFormat(&attrBuf, &childBuf, disk->store, 0, > + false) < 0) > + goto cleanup; > + if (virXMLFormatElement(buf, push ? "target" : "scratch", > + &attrBuf, &childBuf) < 0) > + goto cleanup; > + > + virBufferAdjustIndent(buf, -2); > + virBufferAddLit(buf, "</disk>\n"); > + > + ret = 0; > + > + cleanup: > + virBufferFreeAndReset(&attrBuf); > + virBufferFreeAndReset(&childBuf); > + return ret; > + > +int > +virDomainBackupDefFormat(virBufferPtr buf, virDomainBackupDefPtr def, > + bool internal) > +{ > + size_t i; > + > + virBufferAsprintf(buf, "<domainbackup mode='%s'", > + virDomainBackupTypeToString(def->type)); > + if (def->id) > + virBufferAsprintf(buf, " id='%d'", def->id);> + virBufferAddLit(buf, ">\n"); > + virBufferAdjustIndent(buf, 2); > + > + virBufferEscapeString(buf, "<incremental>%s</incremental>\n", > + def->incremental); > + if (def->server) { > + virBufferAsprintf(buf, "<server transport='%s'", > + virStorageNetHostTransportTypeToString(def->server->transport)); > + virBufferEscapeString(buf, " name='%s'", def->server->name); > + if (def->server->port) > + virBufferAsprintf(buf, " port='%u'", def->server->port); > + virBufferEscapeString(buf, " socket='%s'", def->server->socket); > + virBufferAddLit(buf, "/>\n"); > + } > + > + if (def->ndisks) { > + virBufferAddLit(buf, "<disks>\n"); > + virBufferAdjustIndent(buf, 2); > + for (i = 0; i < def->ndisks; i++) { > + if (!def->disks[i].store) > + continue; > + if (virDomainBackupDiskDefFormat(buf, &def->disks[i], > + def->type == VIR_DOMAIN_BACKUP_TYPE_PUSH, > + internal) < 0) > + return -1; > + } > + virBufferAdjustIndent(buf, -2); > + virBufferAddLit(buf, "</disks>\n"); > + } > + > + virBufferAdjustIndent(buf, -2); > + virBufferAddLit(buf, "</domainbackup>\n"); > + > + return virBufferCheckError(buf); > + > + > +static int > +virDomainBackupCompareDiskIndex(const void *a, const void *b) > +{ > + const virDomainBackupDiskDef *diska = a; > + const virDomainBackupDiskDef *diskb = b; > + > + /* Integer overflow shouldn't be a problem here. */ > + return diska->idx - diskb->idx; > + > +static int > +virDomainBackupDefAssignStore(virDomainBackupDiskDefPtr disk, > + virStorageSourcePtr src, > + const char *suffix) > +{ > + int ret = -1; > + > + if (virStorageSourceIsEmpty(src)) { > + if (disk->store) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("disk '%s' has no media"), disk->name); > + goto cleanup; > + } > + } else if (src->readonly && disk->store) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("backup of readonly disk '%s' makes no sense"), > + disk->name); > + goto cleanup; > + } else if (!disk->store) { > + if (virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_FILE) { > + if (VIR_ALLOC(disk->store) < 0) > + goto cleanup; > + disk->store->type = VIR_STORAGE_TYPE_FILE; > + if (virAsprintf(&disk->store->path, "%s.%s", src->path, > + suffix) < 0) > + goto cleanup; > + disk->store->detected = true; > + } else { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("refusing to generate file name for disk '%s'"), > + disk->name); > + goto cleanup; > + } > + } does there need to be an else here ? e.g. can disk->store be set and @src not assigned there? The caller I assume knows to manage @src afterwards. Since there's no cleanup to be done, we could just return directly. > + > +/* 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. */ Similar locking concerns as previously - that is locking domain object. This is really familiar code too - perhaps some code sharability is possible. VIR_AUTOPTR(virBitmap) map = NULL; (could be useful other times too, but I just noticed it). Trying to picture how this happens... should this use STRNEQ_NULLABLE? But no VIR_FREE(disk->source) how does that play with the following? But is it "hooked up" with hotplug/hotunplug? Beginning to think/wonder why backup/checkpoint isn't a child of storage source. Using autofree stuff means the cleanup is unnecessary and we return directly. John > + return ret; > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index fbe7ba2d40..b9b7684494 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -69,6 +69,13 @@ virCapabilitiesSetNetPrefix; > > > # conf/checkpoint_conf.h > +virDomainBackupAlignDisks; > +virDomainBackupDefFormat; > +virDomainBackupDefFree; > +virDomainBackupDefParseNode; > +virDomainBackupDefParseString; > +virDomainBackupTypeFromString; > +virDomainBackupTypeToString; > virDomainCheckpointAlignDisks; > virDomainCheckpointAssignDef; > virDomainCheckpointDefFormat; > @@ -88,7 +95,6 @@ virDomainCheckpointTypeToString; > virDomainCheckpointUpdateRelations; > virDomainListAllCheckpoints; > > - > # conf/cpu_conf.h > virCPUCacheModeTypeFromString; > virCPUCacheModeTypeToString; >