Re: [libvirt] [PATCH 3/3] introduce pull backup

On 09/07/2016 05:24 AM, Nikolay Shirokovskiy wrote: This patch fails make check/syntax-check : GEN check-augeas-virtlogd --- remote_protocol-structs 2016-09-26 08:01:11.645962427 -0400 +++ remote_protocol-struct-t3 2016-09-26 08:59:57.386094561 -0400 @@ -2080,6 +2080,21 @@ remote_nonnull_domain_snapshot snap; u_int flags; }; +struct remote_domain_backup_start_args { + remote_nonnull_domain dom; + remote_nonnull_string xml_desc; + u_int flags; +}; +struct remote_domain_backup_start_ret { + int result; +}; +struct remote_domain_backup_stop_args { + remote_nonnull_domain dom; + u_int flags; +}; +struct remote_domain_backup_stop_ret { + int result; +}; struct remote_domain_open_console_args { remote_nonnull_domain dom; remote_string dev_name; @@ -3169,4 +3184,6 @@ REMOTE_PROC_CONNECT_NODE_DEVICE_EVENT_DEREGISTER_ANY = 375, REMOTE_PROC_NODE_DEVICE_EVENT_LIFECYCLE = 376, REMOTE_PROC_NODE_DEVICE_EVENT_UPDATE = 377, + REMOTE_PROC_DOMAIN_BACKUP_START = 378, + REMOTE_PROC_DOMAIN_BACKUP_STOP = 379, }; Makefile:11101: recipe for target 'remote_protocol-struct' failed This also needs to be split up and perhaps regenerated as an RFC so that debate can be had on your cover/.0 description. Because it's also dependent upon an x-blockdev-del, it cannot be pushed upstream to libvirt. I know qemu work continues w/r/t blockdev-add and backups, but I don't follow it in detail (not enough time in the day!). I'll scan the rest to provide a few comments... I really didn't dig into algorithms too much. Unnecessary to have so much space between QUIESCE and =... If the comment is to be multi-lined, then just make it before the name and at the same indention level, eg.t /* Longish explanation ... */ VIR_DOMAIN_XXX = ... I know we don't always follow that in existing code, but it's easier to read than 4 lines with only so much space. BTW: Using QUIESCE would then rely on the guest agent being available for the domain... Just making a mental note, but yet something you have a dependency upon. "domain" Very sparse comparatively order... "domain-backup" before "domain-snapshot" (b before s) Use a shorter name, suggest "DOMBKUP" why not domain_backup_conf.{c,h} similarly virsh-domain-backup Try to keep the \ column alignment Similar \ column alignment IPv4, IPv6, IPnotinventedyet why not "tcp", "tls", etc.? That would seemingly allow a bit of code reuse for client transport. There's also virStorageNetHostTransport FYI: also see qemuMonitorGraphicsAddressFamily In the cover/.0 - type is 'incremental' - that doesn't match FILE... This would be an optional attribute doing what? Some more assumptions? Not really clear yet what present does... This would be 'present' not def->name And your <bitmap> from the cover is missing here. Two lines between functions... Typically when I see <address> I think of something very different - as in the address of the controller for a device... > + if (!type) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("backup address type must be specified")); > + > + if ((def->type = virDomainBackupAddressTypeFromString(type)) < 0) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("unknown backup address type '%s'"), type); > + Why differ from existing transport definitions? Seems like there would be code reuse opportunities. BTW: If you're going to have a network protocol, then shouldn't you also have an authentication mechanism? > + switch (def->type) { > + case VIR_DOMAIN_BACKUP_ADDRESS_IP: > + def->data.ip.host = virXMLPropString(node, "host"); > + port = virXMLPropString(node, "port"); > + if (!def->data.ip.host || !port) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("both host and port must be specified " > + "for ip address type")); > + goto cleanup; > + } > + if (virStrToLong_i(port, NULL, 10, &def->data.ip.port) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("cannot parse port %s"), port); > + goto cleanup; > + } > + break; > + case VIR_DOMAIN_BACKUP_ADDRESS_UNIX: > + def->data.socket.path = virXMLPropString(node, "path"); > + if (!def->data.socket.path) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("path must be specified for unix address type")); > + goto cleanup; > + } > + break; > + } > + > + ret = 0; > + > + cleanup: > + VIR_FREE(type); > + VIR_FREE(port); > + > + return ret; > +} > + > +static virDomainBackupDefPtr > +virDomainBackupDefParse(xmlXPathContextPtr ctxt) > +{ > + virDomainBackupDefPtr def = NULL; > + virDomainBackupDefPtr ret = NULL; > + xmlNodePtr *nodes = NULL, node; > + size_t i; > + int n; > + > + if (VIR_ALLOC(def) < 0) > + goto cleanup; > + > + if (!(node = virXPathNode("./address[1]", ctxt))) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("export address must be specifed")); > + > + if (virDomainBackupAddressDefParseXML(node, &def->address) < 0) > + goto cleanup; > + > + if ((n = virXPathNodeSet("./disks/*", ctxt, &nodes)) < 0) > + goto cleanup; So the purpose of disks is what? Should those be "domain_backup"? > + > + ctxt = xmlXPathNewContext(xml); > + if (ctxt == NULL) { > + virReportOOMError(); > + > + ctxt->node = root; > + def = virDomainBackupDefParse(ctxt); > + > + cleanup: > + xmlXPathFreeContext(ctxt); > + > + return def; > +} > + > + > +virDomainBackupDefPtr > +virDomainBackupDefParseString(const char *xmlStr, > + virCapsPtr caps, > + virDomainXMLOptionPtr xmlopt, > + unsigned int flags) > +{ > + virDomainBackupDefPtr ret = NULL; > + xmlDocPtr xml; > + int keepBlanksDefault = xmlKeepBlanksDefault(0); > + > + if ((xml = virXMLParse(NULL, xmlStr, _("(domain_backup)")))) { Consistency with "domainbackup" above? coding style void virDomain... And there's no Format routines to complement Parse... By choice? NULLSTR(xmlDesc) since you check NullArgGoto below... Since 2.2 is already out, you would have created a new stanza, e.g. LIBVIRT_2.3.0 { global: virDomainBackupStart; virDomainBackupStop; } LIBVIRT_2.2.0; add a blank line - for readability. Is this like hiccuping ;-) Maybe consider an enum of sorts for a state of a backup... none, possible, in process, failed, ?? others... Haven't got that far yet. If it's possible to backup, then there's perhaps some sanity checks that need to be made or at least synchronizing with migration. Would be horrible to start a migration and then start a backup. Just thinking off the cuff without looking at the rest of the code. bool ? cannot wait to see this! Not sure I can do the rest of the changes justice as my time to look is running out. The pile is just too large and "sharing" opportunities too great... Then I would think the code could be reused/shared some how... A name selection problem... Seems to me some way to use a temporary file name could be devised. Consider using ".XXXXXX" and then mkostemp (there are other examples). Once you've successfully backed things up the name then changes to whatever user supplied name is provided. And of course similar issues exist there - do you overwrite existing files? You may also end up with a space problem. That is - does the size of your backup exceed the size of the space to be written. Again, I didn't check following code to see if/whether that's true - it just came to mind as another one of those areas we need to consider. We also have to consider the security aspects of creating a file and using security manager for "domain" files (the selinux, apparmor, etc) setup for labels and the such. Wouldn't this only work if we used the same name from within the domain? What if another domain used the ".backup" file name? IOW: This isn't checking if "on disk" right now there isn't a file with the name you just attempted to create. > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("cannot generate backup name for " > + "disk '%s': collision with disk '%s'"), > + disk->name, def->disks[j].name); > + return -1; > + } > + } > + } > + > + return 0; > +} > + > +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 > +virDomainBackupAlignDisks(virDomainBackupDefPtr def) > +{ > + int ret = -1; > + virBitmapPtr map = NULL; > + size_t i; > + int ndisks; > + > + if (!def->dom) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("missing domain in backup")); > + > + if (!(map = virBitmapNew(def->dom->ndisks))) > + goto cleanup; > + > + /* Double check requested disks. */ > + for (i = 0; i < def->ndisks; i++) { I cannot believe the number of times we traverse this array. For 1 or 2 disks - easy... For 100-200 disks, that could be brutal especially error paths... This needs to be a check against VIR_TRISTATE_BOOL_ABSENT to make it obvious... Says something doesn't it? > +static int > +qemuBackupDriveCancelSingle(virQEMUDriverPtr driver, > + virDomainObjPtr vm, > + virDomainDiskDefPtr disk) > +{ > + qemuDomainObjPrivatePtr priv = vm->privateData; > + char *diskAlias = NULL; > + int ret = -1; > + int status; > + int rv; > + > + status = qemuBlockJobUpdate(driver, vm, disk); > + if (status == VIR_DOMAIN_BLOCK_JOB_FAILED) { > + virReportError(VIR_ERR_OPERATION_FAILED, > + _("disk %s backup failed"), disk->dst); > + > + if (!(diskAlias = qemuAliasFromDisk(disk))) > + goto cleanup; > + > + qemuDomainObjEnterMonitor(driver, vm); > + rv = qemuMonitorBlockJobCancel(priv->mon, diskAlias, true); > + /* -2 means race condition, job is already failed but > + * event is not yet delivered, thus we continue as > + * in case of success, next block job update will deliver the event */ Looks like this is where you'd want that error checking code I mentioned in patch 2...... Whether you'd also need to do a virResetLastError is another "choice" to make > + if (qemuDomainObjExitMonitor(driver, vm) < 0 || rv == -1) > + goto cleanup; > + > + ret = 0; > + > + cleanup: > + if (ret < 0) { > + qemuBlockJobSyncEnd(driver, vm, disk); > + QEMU_DOMAIN_DISK_PRIVATE(disk)->backuping = false; > + QEMU_DOMAIN_DISK_PRIVATE(disk)->backupFailed = false; > + } > + > + VIR_FREE(diskAlias); > + > + return ret; > +} > + > +static bool > +qemuBackupDriveCancelled(virQEMUDriverPtr driver, > + virDomainObjPtr vm, > + virErrorPtr *err) > +{ > + size_t i; > + size_t active = 0; > + int status; > + > + *err = NULL; > + > + for (i = 0; i < vm->def->ndisks; i++) { > + virDomainDiskDefPtr disk = vm->def->disks[i]; > + qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); > + > + if (!diskPriv->backuping) > + continue; > + > + status = qemuBlockJobUpdate(driver, vm, disk); > + switch (status) { > + case VIR_DOMAIN_BLOCK_JOB_FAILED: > + virReportError(VIR_ERR_OPERATION_FAILED, > + _("migration of disk %s failed"), disk->dst); > + if (!*err) > + *err = virSaveLastError(); > + /* fallthrough */ > + case VIR_DOMAIN_BLOCK_JOB_CANCELED: > + qemuBlockJobSyncEnd(driver, vm, disk); > + diskPriv->backuping = false; > + diskPriv->backupFailed = false; > + break; > + > + default: > + ++active; > + } > + } > + > + return active == 0; > +} > + > +/** > + * FIXME nearly copy paste from qemuMigrationCancelDriveMirror > + * > + * Cancel backup jobs for all disks > + * > + * Returns 0 on success, -1 otherwise. > + */ > +static int > +qemuBackupDriveCancelAll(virQEMUDriverPtr driver, > + virDomainObjPtr vm) > +{ > + virErrorPtr err = NULL, wait_err; > + size_t i; > + int ret = -1; > + > + VIR_DEBUG("Cancelling drive mirrors for domain %s", vm->def->name); > + > + for (i = 0; i < vm->def->ndisks; i++) { > + virDomainDiskDefPtr disk = vm->def->disks[i]; > + qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); > + > + if (!diskPriv->backuping) > + continue; > + > + if (qemuBackupDriveCancelSingle(driver, vm, disk) < 0) { > + if (!virDomainObjIsActive(vm)) > + goto cleanup; > + > + if (!err) > + err = virSaveLastError(); > + } > + } > + > + while (!qemuBackupDriveCancelled(driver, vm, &wait_err)) { > + if (!err && wait_err) > + err = wait_err; > + else > + virFreeError(wait_err); > + > + if (virDomainObjWait(vm) < 0) > + goto cleanup; > + } > + > + if (!err) > + ret = 0; > + > + cleanup: > + if (err) { > + virSetError(err); > + virFreeError(err); > + } > + > + return ret; > +} > + > +static int > +qemuDomainBackupFinishExport(virQEMUDriverPtr driver, > + virDomainObjPtr vm) > +{ > + qemuDomainObjPrivatePtr priv = vm->privateData; > + virErrorPtr err = NULL; > + char *dev = NULL; > + int ret = -1; > + size_t i; > + > + qemuDomainObjEnterMonitor(driver, vm); > + ignore_value(qemuMonitorNBDServerStop(priv->mon)); > + if (qemuDomainObjExitMonitor(driver, vm) < 0) > + return -1; > + > + if (qemuBackupDriveCancelAll(driver, vm) < 0) { > + if (!virDomainObjIsActive(vm)) > + goto cleanup; > + > + err = virSaveLastError(); > + } > + > + for (i = 0; i < vm->def->ndisks; i++) { > + virDomainDiskDefPtr disk = vm->def->disks[i]; > + qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); > + > + if (!diskPriv->backupdev) > + continue; > + diskPriv->backupdev = false; > + > + if (virAsprintf(&dev, "backup-%s", disk->info.alias) < 0) > + continue; > + > + qemuDomainObjEnterMonitor(driver, vm); > + ignore_value(qemuMonitorBlockdevDel(priv->mon, dev)); > + if (qemuDomainObjExitMonitor(driver, vm) < 0) > + goto cleanup; > + > + VIR_FREE(dev); > + } > + > + if (!err) > + ret = 0; > + cleanup: > + if (err) { > + virSetError(err); > + virFreeError(err); > + } > + VIR_FREE(dev); > + > + return ret; > +} > + > +static int > +qemuDomainBackupPrepareDisk(virQEMUDriverPtr driver, > + virDomainObjPtr vm, > + virStorageSourcePtr src, > + struct qemuDomainDiskInfo *info) > +{ > + char *path = NULL; > + virCommandPtr cmd = NULL; > + int ret = -1; > + const char *qemuImgPath; > + > + if (!(qemuImgPath = qemuFindQemuImgBinary(driver))) > + goto cleanup; > + > + if (qemuGetDriveSourceString(src, NULL, &path) < 0) > + goto cleanup; > + > + if (qemuDomainStorageFileInit(driver, vm, src) < 0) > + goto cleanup; > + > + if (virStorageFileCreate(src) < 0) { > + virReportSystemError(errno, _("failed to create image file '%s'"), > + path); > + > + if (!(cmd = virCommandNewArgList(qemuImgPath, "create", > + "-f", "qcow2", > + NULL))) > + goto cleanup; > + > + virCommandAddArg(cmd, src->path); > + virCommandAddArgFormat(cmd, "%lli", info->guest_size); > + > + if (virCommandRun(cmd, NULL) < 0) > + goto cleanup; > + > + ret = 0; > + > + cleanup: > + virStorageFileDeinit(src); > + VIR_FREE(path); > + virCommandFree(cmd); > + > + return ret; > +} > + > +static int > +qemuDomainBackupExportDisks(virQEMUDriverPtr driver, > + virDomainObjPtr vm, > + virDomainBackupDefPtr def) > +{ > + qemuDomainObjPrivatePtr priv = vm->privateData; > + char *dev = NULL; > + char *dev_backup = NULL; > + int ret = -1, rc; > + virJSONValuePtr actions = NULL; > + size_t i; > + virHashTablePtr block_info = NULL; > + > + qemuDomainObjEnterMonitor(driver, vm); > + rc = qemuMonitorNBDServerStart(priv->mon, def->address.data.ip.host, > + def->address.data.ip.port); Well it may have been really good to note that usage of the NBD server was taking place somewhere earlier. Each of the following goes through ndisks array and does multiple/different things, but it's not clear what error recovery looks like if any one fails - the cleanup path is shall we say sparse while the setup is multiple steps. I haven't compared it to the migration code... For as many times as this shows up - I'm now wondering why isn't this code working off a list of disks where present can be assumed.. This would be leaked for as many times as this is called. Is this where you copied the code from originally? NBD? > + if (def->address.type != VIR_DOMAIN_BACKUP_ADDRESS_IP) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + "%s", _("backup thru unix sockets is not supported")); > + > + if (!virDomainObjIsActive(vm)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("backing up inactive domains is not supported")); > + > + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) > + goto cleanup; > + job = true; Typically the goto's after this point would be a goto endjob type scenario. There's plenty of examples. We seem to be going through the ndisks far too frequently. Something says this check in particular should be done in the AlignDisks helper Seems like more adjustment for code reuse... What if it's already frozen? freezed? frozen? And this doesn't need a QUIESCE check? > + if (freezed != -1 && qemuDomainSnapshotFSThaw(driver, vm, ret == 0) < 0) > + ret = -1; > + > + if (ret < 0 && virDomainObjIsActive(vm)) { > + virErrorPtr err = virSaveLastError(); > + > + ignore_value(qemuDomainBackupFinishExport(driver, vm)); > + virSetError(err); > + virFreeError(err); > + } else if (ret == 0) { > + for (i = 0; i < vm->def->ndisks; i++) { > + virDomainDiskDefPtr disk = vm->def->disks[i]; > + qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); > + > + if (!diskPriv->backuping) > + continue; > + > + qemuBlockJobSyncEnd(driver, vm, disk); > + } > + } > + > + if (job) > + qemuDomainObjEndJob(driver, vm); > + > + virDomainBackupDefFree(def); > + virObjectUnref(caps); > + virDomainObjEndAPI(&vm); > + > + return ret; > +} > + > +static int > +qemuDomainBackupStop(virDomainPtr domain, > + unsigned int flags) > +{ > + virQEMUDriverPtr driver = domain->conn->privateData; > + virDomainObjPtr vm = NULL; > + virErrorPtr err = NULL; > + int ret = -1; > + bool job = false; > + size_t i; > + > + virCheckFlags(0, -1); > + > + if (!(vm = qemuDomObjFromDomain(domain))) > + goto cleanup; > + > + if (virDomainBackupStopEnsureACL(domain->conn, vm->def) < 0) > + goto cleanup; > + > + if (!virDomainObjIsActive(vm)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + "%s", _("inactive domain can't have active backup")); > + > + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) > + goto cleanup; > + job = true; > + > + for (i = 0; i < vm->def->ndisks; i++) { > + virDomainDiskDefPtr disk = vm->def->disks[i]; > + qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); > + > + if (!diskPriv->backuping) > + continue; > + > + /* backup blockjob can fail/cancelled between start and stop */ > + if (!diskPriv->blockjob) { > + diskPriv->backuping = false; > + > + if (diskPriv->backupFailed) { > + virReportError(VIR_ERR_OPERATION_FAILED, > + _("disk %s backup failed"), disk->dst); > + if (!err) > + err = virSaveLastError(); > + diskPriv->backupFailed = false; > + } > + continue; > + } > + > + qemuBlockJobSyncBegin(disk); > + } > + > + if (qemuDomainBackupFinishExport(driver, vm) < 0) > + goto cleanup; > + > + if (!err) > + ret = 0; > + > + cleanup: > + if (job) > + qemuDomainObjEndJob(driver, vm); > + > + virDomainObjEndAPI(&vm); > + if (err) { > + virSetError(err); > + virFreeError(err); > + } > + > + return ret; > +} > > static virHypervisorDriver qemuHypervisorDriver = { > .name = QEMU_DRIVER_NAME, > @@ -20332,6 +21014,8 @@ static virHypervisorDriver qemuHypervisorDriver = { > .domainMigrateStartPostCopy = qemuDomainMigrateStartPostCopy, /* 1.3.3 */ > .domainGetGuestVcpus = qemuDomainGetGuestVcpus, /* 2.0.0 */ > .domainSetGuestVcpus = qemuDomainSetGuestVcpus, /* 2.0.0 */ > + .domainBackupStart = qemuDomainBackupStart, /* 2.3.0 */ > + .domainBackupStop = qemuDomainBackupStop, /* 2.3.0 */ > }; > > > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c > index 944856d..dad61bd 100644 > --- a/src/qemu/qemu_monitor.c > +++ b/src/qemu/qemu_monitor.c > @@ -4001,3 +4001,35 @@ qemuMonitorGetRTCTime(qemuMonitorPtr mon, > > return qemuMonitorJSONGetRTCTime(mon, tm); > } > + int qemuMonitor... and realistically my time as run out. I really didn't look too much in depth at the qemu_driver code. My head was spinning from the number of times going through the ndisks and the inlined code... John > +int qemuMonitorBlockdevBackup(virJSONValuePtr actions, > + const char *device, > + const char *target, > + unsigned long long speed) > +{ > + VIR_DEBUG("actions=%p, device=%s, target=%s, speed=%llu", > + actions, device, target, speed); > + > + return qemuMonitorJSONBlockdevBackup(actions, device, target, speed); > +} > + > +int qemuMonitorBlockdevAdd(qemuMonitorPtr mon, > + const char *id, > + const char *path) > +{ > + VIR_DEBUG("mon=%p, id=%s, path=%s", mon, id, path); > + > + QEMU_CHECK_MONITOR_JSON(mon); > + > + return qemuMonitorJSONBlockdevAdd(mon, id, path); > +} > + > +int qemuMonitorBlockdevDel(qemuMonitorPtr mon, > + const char *id) > +{ > + VIR_DEBUG("mon=%p, id=%s", mon, id); > + > + QEMU_CHECK_MONITOR_JSON(mon); > + > + return qemuMonitorJSONBlockdevDel(mon, id); > +} > diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h > index b838725..3cfd32b 100644 > --- a/src/qemu/qemu_monitor.h > +++ b/src/qemu/qemu_monitor.h > @@ -991,4 +991,16 @@ int qemuMonitorMigrateStartPostCopy(qemuMonitorPtr mon); > int qemuMonitorGetRTCTime(qemuMonitorPtr mon, > struct tm *tm); > > +int qemuMonitorBlockdevAdd(qemuMonitorPtr mon, > + const char *id, > + const char *path); > + > +int qemuMonitorBlockdevDel(qemuMonitorPtr mon, > + const char *id); > + > +int qemuMonitorBlockdevBackup(virJSONValuePtr actions, > + const char *device, > + const char *target, > + unsigned long long speed); > + > #endif /* QEMU_MONITOR_H */ > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c > index 42b05c4..9ee025a 100644 > --- a/src/qemu/qemu_monitor_json.c > +++ b/src/qemu/qemu_monitor_json.c > @@ -831,6 +831,8 @@ qemuMonitorJSONHandleBlockJobImpl(qemuMonitorPtr mon, > type = VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT; > else if (STREQ(type_str, "mirror")) > type = VIR_DOMAIN_BLOCK_JOB_TYPE_COPY; > + else if (STREQ(type_str, "backup")) > + type = VIR_DOMAIN_BLOCK_JOB_TYPE_BACKUP; > > switch ((virConnectDomainEventBlockJobStatus) event) { > case VIR_DOMAIN_BLOCK_JOB_COMPLETED: > @@ -7260,3 +7262,106 @@ qemuMonitorJSONGetHotpluggableCPUs(qemuMonitorPtr mon, > virJSONValueFree(reply); > return ret; > } > + > +int qemuMonitorJSONBlockdevAdd(qemuMonitorPtr mon, > + const char *id, > + const char *path) > +{ > + int ret = -1; > + virJSONValuePtr cmd = NULL; > + virJSONValuePtr args = NULL; > + virJSONValuePtr file = NULL; > + virJSONValuePtr reply = NULL; > + virJSONValuePtr options = NULL; > + > + if (!(cmd = virJSONValueNewObject()) || > + !(args = virJSONValueNewObject()) || > + !(options = virJSONValueNewObject()) || > + !(file = virJSONValueNewObject())) > + goto cleanup; > + > + if (virJSONValueObjectAppendString(file, "driver", "file") < 0 || > + virJSONValueObjectAppendString(file, "filename", path) < 0) > + goto cleanup; > + > + if (virJSONValueObjectAppendString(options, "driver", "qcow2") < 0 || > + virJSONValueObjectAppendString(options, "id", id) < 0 || > + virJSONValueObjectAppend(options, "file", file) < 0) > + goto cleanup; > + > + if (virJSONValueObjectAppend(args, "options", options) < 0) > + goto cleanup; > + > + if (virJSONValueObjectAppendString(cmd, "execute", "blockdev-add") < 0 || > + virJSONValueObjectAppend(cmd, "arguments", args) < 0) > + goto cleanup; > + > + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) > + goto cleanup; > + > + if (qemuMonitorJSONCheckError(cmd, reply) < 0) > + goto cleanup; > + > + ret = 0; > + cleanup: > + virJSONValueFree(cmd); > + virJSONValueFree(args); > + virJSONValueFree(file); > + virJSONValueFree(reply); > + virJSONValueFree(options); > + return ret; > +} > + > +int qemuMonitorJSONBlockdevDel(qemuMonitorPtr mon, > + const char *id) > +{ > + int ret = -1; > + virJSONValuePtr cmd; > + virJSONValuePtr reply = NULL; > + > + cmd = qemuMonitorJSONMakeCommand("x-blockdev-del", > + "s:id", id, > + NULL); > + if (!cmd) > + return -1; > + > + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) > + goto cleanup; > + > + if (qemuMonitorJSONCheckError(cmd, reply) < 0) > + goto cleanup; > + > + ret = 0; > + cleanup: > + virJSONValueFree(cmd); > + virJSONValueFree(reply); > + return ret; > +} > + > +int qemuMonitorJSONBlockdevBackup(virJSONValuePtr actions, > + const char *device, > + const char *target, > + unsigned long long speed) > +{ > + int ret = -1; > + virJSONValuePtr cmd; > + > + cmd = qemuMonitorJSONMakeCommandRaw(true, > + "blockdev-backup", > + "s:device", device, > + "s:target", target, > + "s:sync", "none", > + "Y:speed", speed, > + NULL); > + if (!cmd) > + return -1; > + > + if (virJSONValueArrayAppend(actions, cmd) < 0) > + goto cleanup; > + > + ret = 0; > + cmd = NULL; > + cleanup: > + virJSONValueFree(cmd); > + return ret; > +} > diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h > index 0eab96f..8ae6c1d 100644 > --- a/src/qemu/qemu_monitor_json.h > +++ b/src/qemu/qemu_monitor_json.h > @@ -499,4 +499,20 @@ int qemuMonitorJSONGetHotpluggableCPUs(qemuMonitorPtr mon, > struct qemuMonitorQueryHotpluggableCpusEntry **entries, > size_t *nentries) > ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); > + > +int qemuMonitorJSONBlockdevAdd(qemuMonitorPtr mon, > + const char *id, > + const char *path) > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); > + > +int qemuMonitorJSONBlockdevDel(qemuMonitorPtr mon, > + const char *id) > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); > + > +int qemuMonitorJSONBlockdevBackup(virJSONValuePtr actions, > + const char *device, > + const char *target, > + unsigned long long speed) > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); > + > #endif /* QEMU_MONITOR_JSON_H */ > diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c > index 3b8b796..52e53f5 100644 > --- a/src/remote/remote_driver.c > +++ b/src/remote/remote_driver.c > @@ -8164,6 +8164,8 @@ static virHypervisorDriver hypervisor_driver = { > .domainMigrateStartPostCopy = remoteDomainMigrateStartPostCopy, /* 1.3.3 */ > .domainGetGuestVcpus = remoteDomainGetGuestVcpus, /* 2.0.0 */ > .domainSetGuestVcpus = remoteDomainSetGuestVcpus, /* 2.0.0 */ > + .domainBackupStart = remoteDomainBackupStart, /* 2.3.0 */ > + .domainBackupStop = remoteDomainBackupStop, /* 2.3.0 */ > }; > > static virNetworkDriver network_driver = { > diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x > index 8a8e263..c62ee0e 100644 > --- a/src/remote/remote_protocol.x > +++ b/src/remote/remote_protocol.x > @@ -2679,6 +2679,25 @@ struct remote_domain_snapshot_delete_args { > unsigned int flags; > }; > > +struct remote_domain_backup_start_args { > + remote_nonnull_domain dom; > + remote_nonnull_string xml_desc; > + unsigned int flags; > +}; > + > +struct remote_domain_backup_start_ret { > + int result; > +}; > + > +struct remote_domain_backup_stop_args { > + remote_nonnull_domain dom; > + unsigned int flags; > +}; > + > +struct remote_domain_backup_stop_ret { > + int result; > +}; > + > struct remote_domain_open_console_args { > remote_nonnull_domain dom; > remote_string dev_name; > @@ -5934,5 +5953,17 @@ enum remote_procedure { > * @generate: both > * @acl: none > */ > - REMOTE_PROC_NODE_DEVICE_EVENT_UPDATE = 377 > + REMOTE_PROC_NODE_DEVICE_EVENT_UPDATE = 377, > + > + /** > + * @generate: both > + * @acl: domain:backup > + */ > + REMOTE_PROC_DOMAIN_BACKUP_START = 378, > + > + /** > + * @generate: both > + * @acl: domain:backup > + */ > + REMOTE_PROC_DOMAIN_BACKUP_STOP = 379 > }; > diff --git a/src/util/virerror.c b/src/util/virerror.c > index 1177570..62b5c62 100644 > --- a/src/util/virerror.c > +++ b/src/util/virerror.c > @@ -138,6 +138,7 @@ VIR_ENUM_IMPL(virErrorDomain, VIR_ERR_DOMAIN_LAST, > "Xen XL Config", > > "Perf", > + "Domain Backup", > ) > > > diff --git a/tools/Makefile.am b/tools/Makefile.am > index e7e42c3..1527c5f 100644 > --- a/tools/Makefile.am > +++ b/tools/Makefile.am > @@ -201,6 +201,7 @@ virsh_SOURCES = \ > virsh-secret.c virsh-secret.h \ > virsh-snapshot.c virsh-snapshot.h \ > virsh-volume.c virsh-volume.h \ > + virsh-backup.c virsh-backup.h \ > $(NULL) > > virsh_LDFLAGS = \ > diff --git a/tools/virsh-backup.c b/tools/virsh-backup.c > new file mode 100644 > index 0000000..c5ed972 > --- /dev/null > +++ b/tools/virsh-backup.c > @@ -0,0 +1,150 @@ > +/* > + * virsh-backup.c: Commands to manage domain backup > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library. If not, see > + * <http://www.gnu.org/licenses/>;. > + * > + * Author: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; > + */ > + > +#include <config.h> > +#include "virsh-backup.h" > + > +#include "internal.h" > +#include "virfile.h" > +#include "virsh-domain.h" > +#include "viralloc.h" > + > +#define VIRSH_COMMON_OPT_DOMAIN_FULL \ > + VIRSH_COMMON_OPT_DOMAIN(N_("domain name, id or uuid")) \ > + > +/* > + * "backup-start" command > + */ > +static const vshCmdInfo info_backup_start[] = { > + {.name = "help", > + .data = N_("Start pull backup") > + }, > + {.name = "desc", > + .data = N_("Start pull backup") > + }, > + {.name = NULL} > +}; > + > +static const vshCmdOptDef opts_backup_start[] = { > + VIRSH_COMMON_OPT_DOMAIN_FULL, > + {.name = "xmlfile", > + .type = VSH_OT_DATA, > + .flags = VSH_OFLAG_REQ, > + .help = N_("domain backup XML") > + }, > + {.name = "quiesce", > + .type = VSH_OT_BOOL, > + .help = N_("quiesce guest's file systems") > + }, > + {.name = NULL} > +}; > + > +static bool > +cmdBackupStart(vshControl *ctl, const vshCmd *cmd) > +{ > + virDomainPtr dom = NULL; > + bool ret = false; > + const char *from = NULL; > + char *buffer = NULL; > + unsigned int flags = 0; > + > + if (vshCommandOptBool(cmd, "quiesce")) > + flags |= VIR_DOMAIN_BACKUP_START_QUIESCE; > + > + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) > + goto cleanup; > + > + if (vshCommandOptStringReq(ctl, cmd, "xmlfile", &from) < 0) > + goto cleanup; > + > + if (virFileReadAll(from, VSH_MAX_XML_FILE, &buffer) < 0) { > + vshSaveLibvirtError(); > + > + if (virDomainBackupStart(dom, buffer, flags) < 0) > + goto cleanup; > + > + vshPrint(ctl, _("Domain backup started")); > + ret = true; > + > + cleanup: > + VIR_FREE(buffer); > + if (dom) > + virDomainFree(dom); > + > + return ret; > +} > + > +/* > + * "backup-stop" command > + */ > +static const vshCmdInfo info_backup_stop[] = { > + {.name = "help", > + .data = N_("Stop pull backup") > + }, > + {.name = "desc", > + .data = N_("Stop pull backup") > + }, > + {.name = NULL} > +}; > + > +static const vshCmdOptDef opts_backup_stop[] = { > + VIRSH_COMMON_OPT_DOMAIN_FULL, > + {.name = NULL} > +}; > + > +static bool > +cmdBackupStop(vshControl *ctl, const vshCmd *cmd) > +{ > + virDomainPtr dom = NULL; > + bool ret = false; > + > + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) > + goto cleanup; > + > + if (virDomainBackupStop(dom, 0) < 0) > + goto cleanup; > + > + vshPrint(ctl, _("Domain backup stopped")); > + ret = true; > + > + cleanup: > + if (dom) > + virDomainFree(dom); > + > + return ret; > +} > + > +const vshCmdDef backupCmds[] = { > + {.name = "backup-start", > + .handler = cmdBackupStart, > + .opts = opts_backup_start, > + .info = info_backup_start, > + .flags = 0 > + }, > + {.name = "backup-stop", > + .handler = cmdBackupStop, > + .opts = opts_backup_stop, > + .info = info_backup_stop, > + .flags = 0 > + }, > + {.name = NULL} > +}; > diff --git a/tools/virsh-backup.h b/tools/virsh-backup.h > new file mode 100644 > index 0000000..b765ad7 > --- /dev/null > +++ b/tools/virsh-backup.h > @@ -0,0 +1,28 @@ > +/* > + * virsh-backup.h: Commands to manage domain backup > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library. If not, see > + * <http://www.gnu.org/licenses/>;. > + * > + * Author: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; > + */ > + > +#ifndef VIRSH_BACKUP_H > +# define VIRSH_BACKUP_H > + > +# include "virsh.h" > + > +extern const vshCmdDef backupCmds[]; > + > +#endif /* VIRSH_BACKUP_H */ > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c > index a614512..16a7b4c 100644 > --- a/tools/virsh-domain.c > +++ b/tools/virsh-domain.c > @@ -2528,7 +2528,8 @@ VIR_ENUM_IMPL(virshDomainBlockJob, > N_("Block Pull"), > N_("Block Copy"), > N_("Block Commit"), > - N_("Active Block Commit")) > + N_("Active Block Commit"), > + N_("Block Backup")) > > static const char * > virshDomainBlockJobToString(int type) > diff --git a/tools/virsh.c b/tools/virsh.c > index cb60edc..96e2862 100644 > --- a/tools/virsh.c > +++ b/tools/virsh.c > @@ -71,6 +71,7 @@ > #include "virsh-secret.h" > #include "virsh-snapshot.h" > #include "virsh-volume.h" > +#include "virsh-backup.h" > > /* Gnulib doesn't guarantee SA_SIGINFO support. */ > #ifndef SA_SIGINFO > @@ -919,6 +920,7 @@ static const vshCmdGrp cmdGroups[] = { > {VIRSH_CMD_GRP_NODEDEV, "nodedev", nodedevCmds}, > {VIRSH_CMD_GRP_SECRET, "secret", secretCmds}, > {VIRSH_CMD_GRP_SNAPSHOT, "snapshot", snapshotCmds}, > + {VIRSH_CMD_GRP_BACKUP, "backup", backupCmds}, > {VIRSH_CMD_GRP_STORAGE_POOL, "pool", storagePoolCmds}, > {VIRSH_CMD_GRP_STORAGE_VOL, "volume", storageVolCmds}, > {VIRSH_CMD_GRP_VIRSH, "virsh", virshCmds}, > diff --git a/tools/virsh.h b/tools/virsh.h > index fd552bb..839f36f 100644 > --- a/tools/virsh.h > +++ b/tools/virsh.h > @@ -57,6 +57,7 @@ > # define VIRSH_CMD_GRP_NWFILTER "Network Filter" > # define VIRSH_CMD_GRP_SECRET "Secret" > # define VIRSH_CMD_GRP_SNAPSHOT "Snapshot" > +# define VIRSH_CMD_GRP_BACKUP "Backup" > # define VIRSH_CMD_GRP_HOST_AND_HV "Host and Hypervisor" > # define VIRSH_CMD_GRP_VIRSH "Virsh itself" > >