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

On 09/07/2016 05:24 AM, Nikolay Shirokovskiy wrote: > Add API to start/stop exporting disks for a backup and add qemu > implementation. > > The latter is not complete yet. At least backup disks are not > cleaned up and libvirt restart is not handled. > --- > examples/object-events/event-test.c | 3 + > include/libvirt/libvirt-domain-backup.h | 45 +++ > include/libvirt/libvirt-domain.h | 3 + > include/libvirt/libvirt.h | 1 + > include/libvirt/virterror.h | 1 + > po/POTFILES.in | 2 + > src/Makefile.am | 3 + > src/access/viraccessperm.c | 3 +- > src/access/viraccessperm.h | 6 + > src/conf/backup_conf.c | 295 ++++++++++++++ > src/conf/backup_conf.h | 85 ++++ > src/conf/domain_conf.c | 2 +- > src/driver-hypervisor.h | 11 + > src/libvirt-domain-backup.c | 86 ++++ > src/libvirt_private.syms | 6 + > src/libvirt_public.syms | 2 + > src/qemu/qemu_blockjob.c | 2 + > src/qemu/qemu_conf.h | 1 + > src/qemu/qemu_domain.h | 4 + > src/qemu/qemu_driver.c | 684 ++++++++++++++++++++++++++++++++ > src/qemu/qemu_monitor.c | 32 ++ > src/qemu/qemu_monitor.h | 12 + > src/qemu/qemu_monitor_json.c | 105 +++++ > src/qemu/qemu_monitor_json.h | 16 + > src/remote/remote_driver.c | 2 + > src/remote/remote_protocol.x | 33 +- > src/util/virerror.c | 1 + > tools/Makefile.am | 1 + > tools/virsh-backup.c | 150 +++++++ > tools/virsh-backup.h | 28 ++ > tools/virsh-domain.c | 3 +- > tools/virsh.c | 2 + > tools/virsh.h | 1 + > 33 files changed, 1627 insertions(+), 4 deletions(-) > create mode 100644 include/libvirt/libvirt-domain-backup.h > create mode 100644 src/conf/backup_conf.c > create mode 100644 src/conf/backup_conf.h > create mode 100644 src/libvirt-domain-backup.c > create mode 100644 tools/virsh-backup.c > create mode 100644 tools/virsh-backup.h > 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. > diff --git a/examples/object-events/event-test.c b/examples/object-events/event-test.c > index 730cb8b..08490bb 100644 > --- a/examples/object-events/event-test.c > +++ b/examples/object-events/event-test.c > @@ -829,6 +829,9 @@ blockJobTypeToStr(int type) > > case VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT: > return "active layer block commit"; > + > + case VIR_DOMAIN_BLOCK_JOB_TYPE_BACKUP: > + return "block backup"; > } > > return "unknown"; > diff --git a/include/libvirt/libvirt-domain-backup.h b/include/libvirt/libvirt-domain-backup.h > new file mode 100644 > index 0000000..cd24995 > --- /dev/null > +++ b/include/libvirt/libvirt-domain-backup.h > @@ -0,0 +1,45 @@ > +/* > + * libvirt-domain-backup.h > + * Summary: APIs for management of domain backups > + * Description: Provides APIs for the management of domain backups > + * Author: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; > + * > + * > + * 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/>;. > + */ > + > +#ifndef __VIR_LIBVIRT_DOMAIN_BACKUP_H__ > +# define __VIR_LIBVIRT_DOMAIN_BACKUP_H__ > + > +# ifndef __VIR_LIBVIRT_H_INCLUDES__ > +# error "Don't include this file directly, only use libvirt/libvirt.h" > +# endif > + > +typedef enum { > + VIR_DOMAIN_BACKUP_START_QUIESCE = (1 << 0), /* use guest agent to > + quiesce all mounted > + file systems within > + the domain */ 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.

> +} virDomainBackupStartFlags; > + > +int virDomainBackupStart(virDomainPtr domain, > + const char *xmlDesc, > + unsigned int flags); > + > +int virDomainBackupStop(virDomainPtr domaine, "domain" > + unsigned int flags); > + > + > +#endif /* __VIR_LIBVIRT_DOMAIN_BACKUP_H__ */ > diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h > index 6e0e7fb..f2cb759 100644 > --- a/include/libvirt/libvirt-domain.h > +++ b/include/libvirt/libvirt-domain.h > @@ -2048,6 +2048,9 @@ typedef enum { > /* Active Block Commit (virDomainBlockCommit with flags), job > * exists as long as sync is active */ > > + VIR_DOMAIN_BLOCK_JOB_TYPE_BACKUP = 5, > + /* Block Backup */ > + Very sparse comparatively > # ifdef VIR_ENUM_SENTINELS > VIR_DOMAIN_BLOCK_JOB_TYPE_LAST > # endif > diff --git a/include/libvirt/libvirt.h b/include/libvirt/libvirt.h > index 36f6d60..be0d570 100644 > --- a/include/libvirt/libvirt.h > +++ b/include/libvirt/libvirt.h > @@ -37,6 +37,7 @@ extern "C" { > # include <libvirt/libvirt-host.h> > # include <libvirt/libvirt-domain.h> > # include <libvirt/libvirt-domain-snapshot.h> > +# include <libvirt/libvirt-domain-backup.h> order... "domain-backup" before "domain-snapshot" (b before s) > # include <libvirt/libvirt-event.h> > # include <libvirt/libvirt-interface.h> > # include <libvirt/libvirt-network.h> > diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h > index 2ec560e..c1f8c6c 100644 > --- a/include/libvirt/virterror.h > +++ b/include/libvirt/virterror.h > @@ -131,6 +131,7 @@ typedef enum { > VIR_FROM_XENXL = 64, /* Error from Xen xl config code */ > > VIR_FROM_PERF = 65, /* Error from perf */ > + VIR_FROM_DOMAIN_BACKUP = 66,/* Error from domain backup */ Use a shorter name, suggest "DOMBKUP"

> > # ifdef VIR_ENUM_SENTINELS > VIR_ERR_DOMAIN_LAST > diff --git a/po/POTFILES.in b/po/POTFILES.in > index 25dbc84..4cdeb2f 100644 > --- a/po/POTFILES.in > +++ b/po/POTFILES.in > @@ -18,6 +18,7 @@ src/bhyve/bhyve_driver.c > src/bhyve/bhyve_monitor.c > src/bhyve/bhyve_parse_command.c > src/bhyve/bhyve_process.c > +src/conf/backup_conf.c why not domain_backup_conf.{c,h}

> src/conf/capabilities.c > src/conf/cpu_conf.c > src/conf/device_conf.c > @@ -281,6 +282,7 @@ src/xenconfig/xen_xl.c > src/xenconfig/xen_xm.c > tests/virpolkittest.c > tools/libvirt-guests.sh.in > +tools/virsh-backup.c similarly virsh-domain-backup

> tools/virsh-console.c > tools/virsh-domain-monitor.c > tools/virsh-domain.c > diff --git a/src/Makefile.am b/src/Makefile.am > index 8ee5567..c04e72c 100644 > --- a/src/Makefile.am > +++ b/src/Makefile.am > @@ -219,6 +219,7 @@ DRIVER_SOURCES = \ > libvirt.c libvirt_internal.h \ > libvirt-domain.c \ > libvirt-domain-snapshot.c \ > + libvirt-domain-backup.c \ > libvirt-host.c \ > libvirt-interface.c \ > libvirt-network.c \ > @@ -334,6 +335,7 @@ DOMAIN_CONF_SOURCES = \ > conf/domain_audit.c conf/domain_audit.h \ > conf/domain_nwfilter.c conf/domain_nwfilter.h \ > conf/snapshot_conf.c conf/snapshot_conf.h \ > + conf/backup_conf.c conf/backup_conf.h \ Try to keep the \ column alignment

> conf/numa_conf.c conf/numa_conf.h \ > conf/virdomainobjlist.c conf/virdomainobjlist.h > > @@ -2390,6 +2392,7 @@ libvirt_setuid_rpc_client_la_SOURCES = \ > libvirt.c \ > libvirt-domain.c \ > libvirt-domain-snapshot.c \ > + libvirt-domain-backup.c \ Similar \ column alignment

> libvirt-host.c \ > libvirt-interface.c \ > libvirt-network.c \ > diff --git a/src/access/viraccessperm.c b/src/access/viraccessperm.c > index 0f58290..16216c0 100644 > --- a/src/access/viraccessperm.c > +++ b/src/access/viraccessperm.c > @@ -43,7 +43,8 @@ VIR_ENUM_IMPL(virAccessPermDomain, > "fs_trim", "fs_freeze", > "block_read", "block_write", "mem_read", > "open_graphics", "open_device", "screenshot", > - "open_namespace", "set_time", "set_password"); > + "open_namespace", "set_time", "set_password", > + "backup"); > > VIR_ENUM_IMPL(virAccessPermInterface, > VIR_ACCESS_PERM_INTERFACE_LAST, > diff --git a/src/access/viraccessperm.h b/src/access/viraccessperm.h > index 1817da7..06d5184 100644 > --- a/src/access/viraccessperm.h > +++ b/src/access/viraccessperm.h > @@ -306,6 +306,12 @@ typedef enum { > */ > VIR_ACCESS_PERM_DOMAIN_SET_PASSWORD, > > + /** > + * @desc: Backup domain > + * @message: Backing domain up requires authorization > + */ > + VIR_ACCESS_PERM_DOMAIN_BACKUP, /* Backup domain */ > + > VIR_ACCESS_PERM_DOMAIN_LAST, > } virAccessPermDomain; > > diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c > new file mode 100644 > index 0000000..ed00922 > --- /dev/null > +++ b/src/conf/backup_conf.c > @@ -0,0 +1,295 @@ > +/* > + * backup_conf.c: domain backup XML processing > + * > + * 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 <fcntl.h> > +#include <sys/stat.h> > +#include <sys/time.h> > +#include <unistd.h> > + > +#include "internal.h" > +#include "virbitmap.h" > +#include "virbuffer.h" > +#include "count-one-bits.h" > +#include "datatypes.h" > +#include "domain_conf.h" > +#include "virlog.h" > +#include "viralloc.h" > +#include "netdev_bandwidth_conf.h" > +#include "netdev_vport_profile_conf.h" > +#include "nwfilter_conf.h" > +#include "secret_conf.h" > +#include "backup_conf.h" > +#include "virstoragefile.h" > +#include "viruuid.h" > +#include "virfile.h" > +#include "virerror.h" > +#include "virxml.h" > +#include "virstring.h" > + > +#define VIR_FROM_THIS VIR_FROM_DOMAIN_BACKUP > + > +VIR_LOG_INIT("conf.backup_conf"); > + > +VIR_ENUM_IMPL(virDomainBackupAddress, VIR_DOMAIN_BACKUP_ADDRESS_LAST, > + "ip", 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

> + "unix") > + > +static int > +virDomainBackupDiskDefParseXML(xmlNodePtr node, > + xmlXPathContextPtr ctxt, > + virDomainBackupDiskDefPtr def) > +{ > + int ret = -1; > + char *present = NULL; > + char *type = NULL; > + xmlNodePtr cur; > + xmlNodePtr saved = ctxt->node; > + > + ctxt->node = node; > + > + if ((type = virXMLPropString(node, "type")) && > + virStorageTypeFromString(type) != VIR_STORAGE_TYPE_FILE) { In the cover/.0 - type is 'incremental' - that doesn't match FILE... This would be an optional attribute doing what?

> + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("'file' is the only supported backup source type")); > + goto cleanup; > + } > + > + if ((cur = virXPathNode("./source", ctxt))) { > + if (VIR_ALLOC(def->src) < 0) > + goto cleanup; > + > + def->src->type = VIR_STORAGE_TYPE_FILE; > + def->src->format = VIR_STORAGE_FILE_QCOW2; Some more assumptions?

> + > + if (virDomainDiskSourceParse(cur, ctxt, def->src) < 0) > + goto cleanup; > + } > + > + def->name = virXMLPropString(node, "name"); > + if (!def->name) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("missing name from disk backup element")); > + goto cleanup; > + } > + > + present = virXMLPropString(node, "present"); Not really clear yet what present does...

> + if (present && (def->present = virTristateBoolTypeFromString(present)) <= 0) { > + virReportError(VIR_ERR_XML_ERROR, > + _("Invalid disk '%s' present attribute value"), > + def->name); This would be 'present' not def->name

> + goto cleanup; > + } And your <bitmap> from the cover is missing here.

> + > + ret = 0; > + > + cleanup: > + VIR_FREE(present); > + VIR_FREE(type); > + > + ctxt->node = saved; > + > + return ret; > +} > + Two lines between functions... > +static int > +virDomainBackupAddressDefParseXML(xmlNodePtr node, > + virDomainBackupAddressDefPtr def) > +{ > + char *type = virXMLPropString(node, "type"); > + char *port = NULL; > + int ret = -1; > + 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")); > + goto cleanup; > + } > + > + if ((def->type = virDomainBackupAddressTypeFromString(type)) < 0) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("unknown backup address type '%s'"), type); > + goto cleanup; > + } > + 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")); > + goto cleanup; > + } > + > + if (virDomainBackupAddressDefParseXML(node, &def->address) < 0) > + goto cleanup; > + > + if ((n = virXPathNodeSet("./disks/*", ctxt, &nodes)) < 0) > + goto cleanup; So the purpose of disks is what?

> + > + if (n && VIR_ALLOC_N(def->disks, n) < 0) > + goto cleanup; > + def->ndisks = n; > + for (i = 0; i < def->ndisks; i++) { > + if (virDomainBackupDiskDefParseXML(nodes[i], ctxt, &def->disks[i]) < 0) > + goto cleanup; > + } > + > + ret = def; > + def = NULL; > + > + cleanup: > + VIR_FREE(nodes); > + virDomainBackupDefFree(def); > + > + return ret; > +} > + > +virDomainBackupDefPtr > +virDomainBackupDefParseNode(xmlDocPtr xml, > + xmlNodePtr root, > + virCapsPtr caps ATTRIBUTE_UNUSED, > + virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED, > + unsigned int flags) > +{ > + xmlXPathContextPtr ctxt = NULL; > + virDomainBackupDefPtr def = NULL; > + > + virCheckFlags(0, NULL); > + > + if (!xmlStrEqual(root->name, BAD_CAST "domainbackup")) { > + virReportError(VIR_ERR_XML_ERROR, "%s", _("domainbackup")); Should those be "domain_backup"?

> + goto cleanup; > + } > + > + ctxt = xmlXPathNewContext(xml); > + if (ctxt == NULL) { > + virReportOOMError(); > + goto cleanup; > + } > + > + 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?

> + xmlKeepBlanksDefault(keepBlanksDefault); > + ret = virDomainBackupDefParseNode(xml, xmlDocGetRootElement(xml), > + caps, xmlopt, flags); > + xmlFreeDoc(xml); > + } > + xmlKeepBlanksDefault(keepBlanksDefault); > + > + return ret; > +} > + > +static > +void virDomainBackupAddressDefFree(virDomainBackupAddressDefPtr def) > +{ > + switch (def->type) { > + case VIR_DOMAIN_BACKUP_ADDRESS_IP: > + VIR_FREE(def->data.ip.host); > + break; > + case VIR_DOMAIN_BACKUP_ADDRESS_UNIX: > + VIR_FREE(def->data.socket.path); > + break; > + } > +} > + > +void virDomainBackupDefFree(virDomainBackupDefPtr def) coding style void virDomain... > +{ > + size_t i; > + > + if (!def) > + return; > + > + for (i = 0; i < def->ndisks; i++) { > + virDomainBackupDiskDefPtr disk = &def->disks[i]; > + > + virStorageSourceFree(disk->src); > + VIR_FREE(disk->name); > + } > + VIR_FREE(def->disks); > + > + virDomainBackupAddressDefFree(&def->address); > + > + VIR_FREE(def); > +} And there's no Format routines to complement Parse... By choice?

> diff --git a/src/conf/backup_conf.h b/src/conf/backup_conf.h > new file mode 100644 > index 0000000..1b09647 > --- /dev/null > +++ b/src/conf/backup_conf.h > @@ -0,0 +1,85 @@ > +/* > + * backup_conf.h: domain backup XML processing > + * > + * 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 __BACKUP_CONF_H > +# define __BACKUP_CONF_H > + > +# include "internal.h" > +# include "domain_conf.h" > + > +typedef struct _virDomainBackupDiskDef virDomainBackupDiskDef; > +typedef virDomainBackupDiskDef *virDomainBackupDiskDefPtr; > +struct _virDomainBackupDiskDef { > + char *name; /* name matching the <target dev='...' of the domain */ > + int present; /* enum virTristateBool */ > + int idx; /* index within dom->disks that matches name */ > + virStorageSourcePtr src; > +}; > + > +typedef enum { > + VIR_DOMAIN_BACKUP_ADDRESS_IP, > + VIR_DOMAIN_BACKUP_ADDRESS_UNIX, > + > + VIR_DOMAIN_BACKUP_ADDRESS_LAST, > +} virDomainBackupAddressType; > + > +VIR_ENUM_DECL(virDomainBackupAddress) > + > +typedef struct _virDomainBackupAddressDef virDomainBackupAddressDef; > +typedef virDomainBackupAddressDef *virDomainBackupAddressDefPtr; > +struct _virDomainBackupAddressDef { > + union { > + struct { > + char *host; > + int port; > + } ip; > + struct { > + char *path; > + } socket; > + } data; > + int type; /* virDomainBackupAddress */ > +}; > + > +/* Stores the complete backup metadata */ > +typedef struct _virDomainBackupDef virDomainBackupDef; > +typedef virDomainBackupDef *virDomainBackupDefPtr; > +struct _virDomainBackupDef { > + virDomainBackupAddressDef address; > + > + size_t ndisks; > + virDomainBackupDiskDef *disks; > + > + virDomainDefPtr dom; > +}; > + > +virDomainBackupDefPtr virDomainBackupDefParseString(const char *xmlStr, > + virCapsPtr caps, > + virDomainXMLOptionPtr xmlopt, > + unsigned int flags); > + > +virDomainBackupDefPtr virDomainBackupDefParseNode(xmlDocPtr xml, > + xmlNodePtr root, > + virCapsPtr caps, > + virDomainXMLOptionPtr xmlopt, > + unsigned int flags); > + > +void virDomainBackupDefFree(virDomainBackupDefPtr def); > + > +#endif /* __BACKUP_CONF_H */ > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index c8c4f61..f0247e2 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -838,7 +838,7 @@ VIR_ENUM_IMPL(virDomainLoader, > * <mirror> XML (remaining types are not two-phase). */ > VIR_ENUM_DECL(virDomainBlockJob) > VIR_ENUM_IMPL(virDomainBlockJob, VIR_DOMAIN_BLOCK_JOB_TYPE_LAST, > - "", "", "copy", "", "active-commit") > + "", "", "copy", "", "active-commit", "") > > VIR_ENUM_IMPL(virDomainMemoryModel, VIR_DOMAIN_MEMORY_MODEL_LAST, > "", "dimm") > diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h > index 5cd1fdf..63ada62 100644 > --- a/src/driver-hypervisor.h > +++ b/src/driver-hypervisor.h > @@ -1251,6 +1251,15 @@ typedef int > int state, > unsigned int flags); > > +typedef int > +(*virDrvDomainBackupStart)(virDomainPtr domain, > + const char *xmlDesc, > + unsigned int flags); > + > +typedef int > +(*virDrvDomainBackupStop)(virDomainPtr domain, > + unsigned int flags); > + > typedef struct _virHypervisorDriver virHypervisorDriver; > typedef virHypervisorDriver *virHypervisorDriverPtr; > > @@ -1489,6 +1498,8 @@ struct _virHypervisorDriver { > virDrvDomainMigrateStartPostCopy domainMigrateStartPostCopy; > virDrvDomainGetGuestVcpus domainGetGuestVcpus; > virDrvDomainSetGuestVcpus domainSetGuestVcpus; > + virDrvDomainBackupStart domainBackupStart; > + virDrvDomainBackupStop domainBackupStop; > }; > > > diff --git a/src/libvirt-domain-backup.c b/src/libvirt-domain-backup.c > new file mode 100644 > index 0000000..e4b8a7b > --- /dev/null > +++ b/src/libvirt-domain-backup.c > @@ -0,0 +1,86 @@ > +/* > + * libvirt-domain-backup.c: entry points for virDomainBackupPtr APIs > + * > + * 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 "datatypes.h" > +#include "virlog.h" > + > +VIR_LOG_INIT("libvirt.domain-backup"); > + > +#define VIR_FROM_THIS VIR_FROM_DOMAIN_BACKUP > + > +int > +virDomainBackupStart(virDomainPtr domain, > + const char *xmlDesc, > + unsigned int flags) > +{ > + virConnectPtr conn; > + > + VIR_DOMAIN_DEBUG(domain, "xmlDesc=%s, flags=%x", xmlDesc, flags); NULLSTR(xmlDesc) since you check NullArgGoto below... > + > + virResetLastError(); > + > + virCheckDomainReturn(domain, -1); > + conn = domain->conn; > + > + virCheckNonNullArgGoto(xmlDesc, error); > + virCheckReadOnlyGoto(conn->flags, error); > + > + if (conn->driver->domainBackupStart) { > + if (conn->driver->domainBackupStart(domain, xmlDesc, flags) < 0) > + goto error; > + > + return 0; > + } > + > + virReportUnsupportedError(); > + error: > + virDispatchError(conn); > + return -1; > +} > + > +int > +virDomainBackupStop(virDomainPtr domain, > + unsigned int flags) > +{ > + virConnectPtr conn; > + > + VIR_DOMAIN_DEBUG(domain, "flags=%x", flags); > + > + virResetLastError(); > + > + virCheckDomainReturn(domain, -1); > + conn = domain->conn; > + > + virCheckReadOnlyGoto(conn->flags, error); > + > + if (conn->driver->domainBackupStop) { > + if (conn->driver->domainBackupStop(domain, flags) < 0) > + goto error; > + > + return 0; > + } > + > + virReportUnsupportedError(); > + error: > + virDispatchError(conn); > + return -1; > +} > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index d62c74c..e0ae661 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -42,6 +42,12 @@ virAccessPermStorageVolTypeFromString; > virAccessPermStorageVolTypeToString; > > > +# conf/backup_conf.h > +virDomainBackupDefFree; > +virDomainBackupDefParseNode; > +virDomainBackupDefParseString; > + > + > # conf/capabilities.h > virCapabilitiesAddGuest; > virCapabilitiesAddGuestDomain; > diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms > index e01604c..8ea164e 100644 > --- a/src/libvirt_public.syms > +++ b/src/libvirt_public.syms > @@ -744,6 +744,8 @@ LIBVIRT_2.2.0 { > global: > virConnectNodeDeviceEventRegisterAny; > virConnectNodeDeviceEventDeregisterAny; > + virDomainBackupStart; > + virDomainBackupStop; > } LIBVIRT_2.0.0; 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; > > # .... define new API here using predicted next version number .... > diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c > index 83a5a3f..66e4ea7 100644 > --- a/src/qemu/qemu_blockjob.c > +++ b/src/qemu/qemu_blockjob.c > @@ -171,6 +171,8 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver, > break; > > case VIR_DOMAIN_BLOCK_JOB_FAILED: > + if (diskPriv->backuping) > + diskPriv->backupFailed = true; add a blank line - for readability. > case VIR_DOMAIN_BLOCK_JOB_CANCELED: > virStorageSourceFree(disk->mirror); > disk->mirror = NULL; > diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h > index 510cd9a..43f85bf 100644 > --- a/src/qemu/qemu_conf.h > +++ b/src/qemu/qemu_conf.h > @@ -32,6 +32,7 @@ > # include "network_conf.h" > # include "domain_conf.h" > # include "snapshot_conf.h" > +# include "backup_conf.h" > # include "domain_event.h" > # include "virthread.h" > # include "security/security_manager.h" > diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h > index ea57111..527ecb0 100644 > --- a/src/qemu/qemu_domain.h > +++ b/src/qemu/qemu_domain.h > @@ -284,6 +284,10 @@ struct _qemuDomainDiskPrivate { > * single disk */ > bool blockjob; > > + bool backuping; 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 backupdev; bool ? cannot wait to see this!

> + bool backupFailed; > + > /* for some synchronous block jobs, we need to notify the owner */ > int blockJobType; /* type of the block job from the event */ > int blockJobStatus; /* status of the finished block job */ > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 807e06d..9b0cb12 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -20118,6 +20118,688 @@ qemuDomainSetGuestVcpus(virDomainPtr dom, > return ret; > } > 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... > +/** > + * FIXME nearly copy-paste of virDomainSnapshotDefAssignExternalNames > + * > + */ Then I would think the code could be reused/shared some how...

> +static int > +virDomainBackupDefGeneratePaths(virDomainBackupDefPtr def) > +{ > + char *tmppath; > + char *tmp; > + size_t i; > + size_t j; > + > + for (i = 0; i < def->ndisks; i++) { > + virDomainBackupDiskDefPtr disk = &def->disks[i]; > + > + if (disk->present != VIR_TRISTATE_BOOL_YES || disk->src) > + continue; > + > + if (VIR_ALLOC(disk->src) < 0) > + return -1; > + > + disk->src->type = VIR_STORAGE_TYPE_FILE; > + disk->src->format = VIR_STORAGE_FILE_QCOW2; > + > + if (VIR_STRDUP(tmppath, virDomainDiskGetSource(def->dom->disks[i])) < 0) > + return -1; > + > + /* drop suffix of the file name */ > + if ((tmp = strrchr(tmppath, '.')) && !strchr(tmp, '/')) > + *tmp = '\0'; > + > + if (virAsprintf(&disk->src->path, "%s.backup", tmppath) < 0) { 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.

> + VIR_FREE(tmppath); > + return -1; > + } > + > + VIR_FREE(tmppath); > + > + /* verify that we didn't generate a duplicate name */ > + for (j = 0; j < i; j++) { > + if (STREQ_NULLABLE(disk->src->path, def->disks[j].src->path)) { 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")); > + goto cleanup; > + } > + > + 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...

> + virDomainBackupDiskDefPtr disk = &def->disks[i]; > + int idx = virDomainDiskIndexByName(def->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, def->dom->disks[idx]->dst)) { > + VIR_FREE(disk->name); > + if (VIR_STRDUP(disk->name, def->dom->disks[idx]->dst) < 0) > + goto cleanup; > + } > + > + if (!disk->present) This needs to be a check against VIR_TRISTATE_BOOL_ABSENT to make it obvious...

> + disk->present = VIR_TRISTATE_BOOL_YES; > + > + if (virStorageSourceIsEmpty(def->dom->disks[i]->src) && > + disk->present == VIR_TRISTATE_BOOL_YES) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("disk '%s' has no source, can not be exported"), > + disk->name); > + } > + } > + > + /* Provide defaults for all remaining disks. */ > + ndisks = def->ndisks; > + if (VIR_EXPAND_N(def->disks, def->ndisks, > + def->dom->ndisks - def->ndisks) < 0) > + goto cleanup; > + > + for (i = 0; i < def->dom->ndisks; i++) { > + virDomainBackupDiskDefPtr disk; > + > + if (virBitmapIsBitSet(map, i)) > + continue; > + > + disk = &def->disks[ndisks++]; > + if (VIR_STRDUP(disk->name, def->dom->disks[i]->dst) < 0) > + goto cleanup; > + disk->idx = i; > + > + if (virStorageSourceIsEmpty(def->dom->disks[i]->src) || > + def->dom->disks[i]->src->readonly) > + disk->present = VIR_TRISTATE_BOOL_NO; > + else > + disk->present = VIR_TRISTATE_BOOL_YES; > + } > + > + qsort(&def->disks[0], def->ndisks, sizeof(def->disks[0]), > + virDomainBackupCompareDiskIndex); > + > + if (virDomainBackupDefGeneratePaths(def) < 0) > + goto cleanup; > + > + ret = 0; > + > + cleanup: > + virBitmapFree(map); > + return ret; > +} > + > + > +/* > + * FIXME has much in common with qemuMigrationCancelOneDriveMirror > + */ 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); > + goto cleanup; > + } > + > + 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); > + goto cleanup; > + } > + > + 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.

> + if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) > + return -1; > + > + if (!(actions = virJSONValueNewArray())) > + goto cleanup; > + > + qemuDomainObjEnterMonitor(driver, vm); > + block_info = qemuMonitorGetBlockInfo(priv->mon); > + if (qemuDomainObjExitMonitor(driver, vm) < 0 || !block_info) > + goto cleanup; > + 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 (i = 0; i < vm->def->ndisks; i++) { > + virDomainDiskDefPtr disk = vm->def->disks[i]; > + struct qemuDomainDiskInfo *disk_info; > + > + if (def->disks[i].present != VIR_TRISTATE_BOOL_YES) > + continue; 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..

> + > + if (!(disk_info = virHashLookup(block_info, disk->info.alias))) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("missing block info for '%s'"), disk->info.alias); > + goto cleanup; > + } > + > + if (qemuDomainBackupPrepareDisk(driver, vm, def->disks[i].src, > + disk_info) < 0) > + goto cleanup; > + > + if (virAsprintf(&dev, "drive-%s", disk->info.alias) < 0) > + goto cleanup; > + if (virAsprintf(&dev_backup, "backup-%s", disk->info.alias) < 0) > + goto cleanup; > + > + qemuDomainObjEnterMonitor(driver, vm); > + rc = qemuMonitorBlockdevAdd(priv->mon, dev_backup, > + def->disks[i].src->path); > + if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) > + goto cleanup; > + > + QEMU_DOMAIN_DISK_PRIVATE(disk)->backupdev = true; > + > + qemuDomainObjEnterMonitor(driver, vm); > + rc = qemuMonitorBlockdevBackup(actions, dev, dev_backup, 0); > + if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) > + goto cleanup; > + > + VIR_FREE(dev); > + VIR_FREE(dev_backup); > + } > + > + qemuDomainObjEnterMonitor(driver, vm); > + rc = qemuMonitorTransaction(priv->mon, actions); > + if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) > + goto cleanup; > + > + for (i = 0; i < vm->def->ndisks; i++) { > + virDomainDiskDefPtr disk = vm->def->disks[i]; > + > + if (def->disks[i].present != VIR_TRISTATE_BOOL_YES) > + continue; > + > + QEMU_DOMAIN_DISK_PRIVATE(disk)->blockjob = true; > + QEMU_DOMAIN_DISK_PRIVATE(disk)->backuping = true; > + qemuBlockJobSyncBegin(disk); > + } > + > + for (i = 0; i < vm->def->ndisks; i++) { > + virDomainDiskDefPtr disk = vm->def->disks[i]; > + > + if (def->disks[i].present != VIR_TRISTATE_BOOL_YES) > + continue; > + > + if (virAsprintf(&dev, "drive-%s", disk->info.alias) < 0) > + goto cleanup; This would be leaked for as many times as this is called.

> + > + qemuDomainObjEnterMonitor(driver, vm); > + rc = qemuMonitorNBDServerAdd(priv->mon, dev, false); > + if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) > + goto cleanup; > + } > + > + ret = 0; > + > + cleanup: > + virJSONValueFree(actions); > + VIR_FREE(dev); > + VIR_FREE(dev_backup); > + virHashFree(block_info); > + > + return ret; > +} > + > +static int > +qemuDomainBackupStart(virDomainPtr domain, > + const char *xmlDesc, > + unsigned int flags) > +{ > + virQEMUDriverPtr driver = domain->conn->privateData; > + virCapsPtr caps = NULL; > + virDomainBackupDefPtr def = NULL; > + virDomainObjPtr vm = NULL; > + int ret = -1; > + bool job = false; > + int freezed = -1; > + size_t i; > + > + virCheckFlags(VIR_DOMAIN_BACKUP_START_QUIESCE, > + -1); > + > + if (!(vm = qemuDomObjFromDomain(domain))) > + goto cleanup; > + > + if (virDomainBackupStartEnsureACL(domain->conn, vm->def) < 0) > + goto cleanup; > + > + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) > + goto cleanup; > + > + if (!(def = virDomainBackupDefParseString(xmlDesc, caps, driver->xmlopt, 0))) > + goto cleanup; > + > + /* FIXME refactor start nbd monitor function to support unix sockets */ 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")); > + goto cleanup; > + } > + > + if (!virDomainObjIsActive(vm)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("backing up inactive domains is not supported")); > + goto cleanup; > + } > + > + 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.

> + > + def->dom = vm->def; > + > + if (virDomainBackupAlignDisks(def) < 0) > + goto cleanup; > + > + for (i = 0; i < vm->def->ndisks; i++) { > + virDomainDiskDefPtr disk = vm->def->disks[i]; > + qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); > + > + if (def->disks[i].present != VIR_TRISTATE_BOOL_YES) > + continue; > + > + if (diskPriv->blockjob) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("disk '%s' already has a blockjob"), disk->dst); > + goto cleanup; We seem to be going through the ndisks far too frequently. Something says this check in particular should be done in the AlignDisks helper

> + } > + } > + > + /* FIXME remove snapshot from below function name */ Seems like more adjustment for code reuse... What if it's already frozen? > + if ((flags & VIR_DOMAIN_BACKUP_START_QUIESCE) && > + (freezed = qemuDomainSnapshotFSFreeze(driver, vm, NULL, 0)) < 0) freezed? frozen?

> + goto cleanup; > + > + if (qemuDomainBackupExportDisks(driver, vm, def) < 0) > + goto cleanup; > + > + ret = 0; > + > + cleanup: > + /* FIXME remove snapshot from name below */ 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")); > + goto cleanup; > + } > + > + 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(); > + goto cleanup; > + } > + > + 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" > >