On 2/12/19 12:08 PM, John Ferlan wrote:
>
>
> On 2/6/19 2:18 PM, Eric Blake wrote:
>> Accept XML describing a generic block job, and output it again
>> as needed. At the moment, it has some qemu-specific hacks, such
>> as storing internal XML for a node name, that might be cleaner
>> once full-tree node-name support goes in.
>>
>> Still not done: decent tests
>>
>> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
>> ---
>> src/conf/checkpoint_conf.h | 65 +++++
>> src/conf/domain_conf.h | 3 +
>> src/conf/checkpoint_conf.c | 503 +++++++++++++++++++++++++++++++++++++
>> src/libvirt_private.syms | 8 +-
>> 4 files changed, 578 insertions(+), 1 deletion(-)
>>
>
> I think this should be it's own module src/conf/backup_conf.{c,h}
That one makes sense to me. Even if the public API lives directly in
libvirt-domain.c (instead of my current placement in
libvirt-domain-checkpoint.c), the backend for this XML is sufficient
enough for its own file. Will split.
>> +
>> + /* Needed? A way for users to list a disk and explicitly mark it
>> + * as not participating, and then output shows all disks rather
>> + * than just active disks */
>> +#if 0
>> + backup = virXMLPropString(node, "backup");
>> + if (backup) {
>> + def->type = virDomainCheckpointTypeFromString(checkpoint);
>> + if (def->type <= 0) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("unknown disk checkpoint setting '%s'"),
>> + checkpoint);
>> + goto cleanup;
>> + }
>> + }
>> +#endif
>
> Still need to decide...
Yes, I do.
>
>> +
>> + if ((type = virXMLPropString(node, "type"))) {
>> + if ((def->store->type = virStorageTypeFromString(type)) <= 0 ||
>> + def->store->type == VIR_STORAGE_TYPE_VOLUME ||
>> + def->store->type == VIR_STORAGE_TYPE_DIR) {
>> + virReportError(VIR_ERR_XML_ERROR,
>> + _("unknown disk backup type '%s'"), type);
>> + goto cleanup;
>> + }
>> + } else {
>> + def->store->type = VIR_STORAGE_TYPE_FILE;
>> + }
>> +
>> + if ((cur = virXPathNode(push ? "./target" : "./scratch", ctxt)) &&
>> + virDomainDiskSourceParse(cur, ctxt, def->store, 0, xmlopt) < 0)
>> + goto cleanup;
>> +
>> + if (internal) {
>
> Is this another way of using FLAGS to determine the parse mode? IOW, is
> this only needed when parsing the status/running XML.
>
Yes, it is for internal-use XML, and I already know I need to fix things
to use a single 'flags' argument with sane enum values (matching what I
already cleaned up for snapshots).
>> +
>> + if (flags & VIR_DOMAIN_BACKUP_PARSE_INTERNAL) {
>> + char *tmp = virXMLPropString(ctxt->node, "id");
>
> You can use VIR_AUTOFREE(char *) tmp too...
>
> Ahh... so this is where it's referenced... I recall this from RNG...
Which also raises the question of whether the RNG has to call it out, if
the only thing using it is internal code, or if the user will ever see
it. We have the interesting problem of outputting some things as
user-visible which are output-only, but then having to validate them in
the RNG on reparse even if we are going to ignore them, so that the user
doesn't have to trim them out. I'll have to double-check my intent
here, and will leave an appropriate comment (probably along the lines of
"id is valid in output and hence part of the RNG, but ignored on input
except when parsing internal XML for reconstructing job state across
libvirtd restarts").
>> + if (node) {
>> + if (def->type != VIR_DOMAIN_BACKUP_TYPE_PULL) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("use of <server> requires pull mode backup"));
>> + goto cleanup;
>> + }
>> + if (VIR_ALLOC(def->server) < 0)
>> + goto cleanup;
>> + if (virDomainStorageNetworkParseHost(node, def->server) < 0)
>> + goto cleanup;
>> + if (def->server->transport == VIR_STORAGE_NET_HOST_TRANS_RDMA) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("transport rdma is not supported for <server>"));
>
> 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...
Good call.
>> +static int
>> +virDomainBackupDiskDefFormat(virBufferPtr buf,
>> + virDomainBackupDiskDefPtr disk,
>> + bool push, bool internal)
>
> one arg per line
>
> Should @internal make use of flags instead?
Yes, especially since I just fixed that in snapshots.
>
>> +{
>> + int type = disk->store->type;
>> + virBuffer attrBuf = VIR_BUFFER_INITIALIZER;
>> + virBuffer childBuf = VIR_BUFFER_INITIALIZER;
>> + int ret = -1;
>> +
>> + if (!disk->name)
>> + return 0;
>> +
>> + virBufferEscapeString(buf, "<disk name='%s'", disk->name);
>> + /* TODO: per-disk backup=off? */
>
> ... I assume this is the <backup> noted earlier.
Yes, matches the earlier #if 0 code where I still need to make a decision.
>
>> +
>> + virBufferAsprintf(buf, " type='%s'>\n", virStorageTypeToString(type));
>> + virBufferAdjustIndent(buf, 2);
>> +
>> + if (disk->store->format > 0)
>> + virBufferEscapeString(buf, "<driver type='%s'/>\n",
>> + virStorageFileFormatTypeToString(disk->store->format));
>> + /* TODO: should node names be part of storage file xml, rather
>> + * than a one-off hack for qemu? */
>
> 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.
I've already had quite a bit of a battle making checkpoints work sanely
across qemu restarts (let alone libvirtd restarts); v5 already has some
different code here. Basically, libvirt has to recompute the node name
any time it connects to a new qemu instance (well, new compared to that
libvirtd process), but caching the node names internally is useful. This
may clean up somewhat if I rebase on top of Peter's blockdev work, but
I'm also trying to make my approaches work without being stuck waiting
for his work to land. (Ideally, when blockdev DOES land, the API parts
are all stable, and only the internals have to change - so I _really_
need to make sure that no XML locks us in to a particular node name
being stored on disk, as that might be incompatible with blockdev
refactoring down the road).
>> +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.
Remember the big #if 0? This is all a hack of using 'disk->store' as a
boolean on whether to visit the disk at the same time as using it as a
pointer to what disk storage to use when it IS being backed up.
Splitting things into a separate flag field may make the code easier to
reason about.
>
>> + ret = 0;
>> + cleanup:
>> + return ret;
>
> 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.
Yes, now all threee of snapshots, checkpoints, and backups have some
form of a AlignDisks function. I'll see if I can come up with a good
split (separating the sorting part that determines how the user's input
maps to the domain, from the validation part that fills in defaults for
anything the user didn't supply explicitly).
>
>> +int
>> +virDomainBackupAlignDisks(virDomainBackupDefPtr def, virDomainDefPtr dom,
>> + const char *suffix)
>> +{
>> + int ret = -1;
>> + virBitmapPtr map = NULL;
>> + size_t i;
>> + int ndisks;
>> + bool alloc_all = false;
>
> VIR_AUTOPTR(virBitmap) map = NULL;
>
> (could be useful other times too, but I just noticed it).
>
>> +
>> + 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)) {
>
> Trying to picture how this happens... should this use STRNEQ_NULLABLE?
No, both disk->name and dom->disks[idx]->dst are non-NULL as part of
their XML parse.
>
>> + 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);
>
> But no VIR_FREE(disk->source) how does that play with the following?
>
>> + disk->store = NULL;
>> + }
>> + if (virDomainBackupDefAssignStore(disk, dom->disks[i]->src, suffix) < 0)
>> + goto cleanup;
>> + }
>> +
>> + /* Provide fillers for all remaining disks, for easier iteration. */
>
> But is it "hooked up" with hotplug/hotunplug?
>
> Beginning to think/wonder why backup/checkpoint isn't a child of storage
> source.
>
Storage source visits only one disk, while snapshot/checkpoint visit all
of the domain's disks at once. But you are also right that having
storage sources be smarter about things may help.
>
>> + if (!def->ndisks)
>> + alloc_all = true;
>> + ndisks = def->ndisks;
>> + if (VIR_EXPAND_N(def->disks, def->ndisks,
>> + dom->ndisks - def->ndisks) < 0)
>> + goto cleanup;
>> +
>> + for (i = 0; i < dom->ndisks; i++) {
>> + virDomainBackupDiskDefPtr disk;
>> +
>> + if (virBitmapIsBitSet(map, i))
>> + continue;
>> + disk = &def->disks[ndisks++];
>> + if (VIR_STRDUP(disk->name, dom->disks[i]->dst) < 0)
>> + goto cleanup;
>> + disk->idx = i;
>> + if (alloc_all &&
>> + virDomainBackupDefAssignStore(disk, dom->disks[i]->src, suffix) < 0)
>> + goto cleanup;
>> + }
>> +
>> + qsort(&def->disks[0], def->ndisks, sizeof(def->disks[0]),
>> + virDomainBackupCompareDiskIndex);
>> +
>> + ret = 0;
>> +
>> + cleanup:
>> + virBitmapFree(map);
>
> 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;
>>
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
