On 26.09.2016 23:07, John Ferlan wrote:

On 09/07/2016 05:24 AM, Nikolay Shirokovskiy wrote:

...

--- src/qemu/qemu_domain.h | 1 + src/qemu/qemu_monitor_json.c | 17 +++++++++++++++++ tests/qemumonitorjsontest.c | 36 ++++++++++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+)

diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 13c0372..ea57111 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -337,6 +337,7 @@ struct qemuDomainDiskInfo { bool tray_open; bool empty; int io_status; + unsigned long long guest_size;

a/k/a: 'virtual-size' in qemu and 'capacity' in libvirt

I probably would stick with capacity or something that says "disk" rather than "guest".

...

};

Trying to determine/decide whether this patch should be "separated" and perhaps made usable with/for the existing callers or whether patch 3 should use the block stats (qemuDomainGetStatsBlock) which already handles some details that could be missing here (at least w/r/t backing chains).

I donno, does backing chain changes anything in this case? I need virtual size of top image only and anyway is it possible for images of backing chain to have different virtual sizes? It does not make much sense. qemuDomainGetStatsBlock (and more specifically qemuMonitorBlockStatsUpdateCapacity which is more suited to get just size) has rather inconviniet arguments more suited for many disks requests.

Another option is adjusting the qemuMonitorJSONDiskNameLookup code to be more multi-purpose since it's using the "query-block" for *BlockPullCommon (from Rebase and Pull) as well as from *BlockCommit and it already checks/expects both "inserted" and "image" to be present in order to return that name.

I would not make function with such specific name provide additionally size info... I think it is perfectily fine to have many monitor commands that internally use same 'query-block' qemu command because this command is really profound)

...

typedef struct _qemuDomainHostdevPrivate qemuDomainHostdevPrivate; diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 3d0a120..7903b47 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1800,6 +1800,8 @@ int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon,

for (i = 0; i < virJSONValueArraySize(devices); i++) { virJSONValuePtr dev = virJSONValueArrayGet(devices, i); + virJSONValuePtr inserted; + virJSONValuePtr image; struct qemuDomainDiskInfo *info; const char *thisdev; const char *status; @@ -1854,6 +1856,21 @@ int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon, if (info->io_status < 0) goto cleanup; } + + if ((inserted = virJSONValueObjectGetObject(dev, "inserted"))) { + if (!(image = virJSONValueObjectGetObject(inserted, "image"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot read 'inserted/image' value")); + goto cleanup; + }

Other code that checks "inserted" and "image" doesn't fail - it just ignores. If there's only going to be one consumer, then I don't believe we want a failure such as this to affect other callers that may not care. That could mean having some sort of "marker" in the returned buffer that the fetch of "virtual-size" did occur (or just using not zero). It would be a shame to have some unexpected failures for a field that nothing else uses especially since the *GetBlockInfo is being used during process launch and reconnect (via qemuProcessRefreshDisks).

I doubt there can be 'inserted' without 'image', it doesn't make sense, inserted what?)) I guess qemuDomainGetStatsBlock ignores missing 'image' because it can afford it, while qemuMonitorJSONDiskNameLookup does not really ignores, if name is not found then function return error eventually. So if this is true, there should be no 'inserted' without 'image', then shame will go to qemu)

Futhermore, what if there's a "backing-image" for "this" particular path? Will that be important for the pull backups support? Without looking at patch 3 I would think so...

Not really. Backing chain has nothing to do with backup, there is just no difference from backup POV. Nikolay

On 28.09.2016 00:53, John Ferlan wrote:

On 09/27/2016 04:17 AM, Nikolay Shirokovskiy wrote:

...

On 26.09.2016 23:07, John Ferlan wrote:

...

On 09/07/2016 05:24 AM, Nikolay Shirokovskiy wrote:

...

--- src/qemu/qemu_domain.h | 1 + src/qemu/qemu_monitor_json.c | 17 +++++++++++++++++ tests/qemumonitorjsontest.c | 36 ++++++++++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+)

diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 13c0372..ea57111 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -337,6 +337,7 @@ struct qemuDomainDiskInfo { bool tray_open; bool empty; int io_status; + unsigned long long guest_size;

a/k/a: 'virtual-size' in qemu and 'capacity' in libvirt

I probably would stick with capacity or something that says "disk" rather than "guest".

...

};

Trying to determine/decide whether this patch should be "separated" and perhaps made usable with/for the existing callers or whether patch 3 should use the block stats (qemuDomainGetStatsBlock) which already handles some details that could be missing here (at least w/r/t backing chains).

I donno, does backing chain changes anything in this case? I need virtual size of top image only and anyway is it possible for images of backing chain to have different virtual sizes? It does not make much sense.

I agree... I don't have in depth knowledge of how the backing chains work, so it's more of a question. Hopefully someone that understands that logic (pkrempa? eblake?) would be able to be more definitive.

I just wouldn't want something to be missed if backing chains were required.

...

qemuDomainGetStatsBlock (and more specifically qemuMonitorBlockStatsUpdateCapacity which is more suited to get just size) has rather inconviniet arguments more suited for many disks requests.

There's lots of similarities that I see in that code... I spent some time today working through a mechanism to make one "query-block" call that 3 consumers (BlockInfo, BlockStats, and DiskNameLookup) could use (I have it working nominally at least). I would think this could then be used to more easily add a new lookup function that's only returning the data you want rather than putting stats data in an BlockInfo...

...

...

Another option is adjusting the qemuMonitorJSONDiskNameLookup code to be more multi-purpose since it's using the "query-block" for *BlockPullCommon (from Rebase and Pull) as well as from *BlockCommit and it already checks/expects both "inserted" and "image" to be present in order to return that name.

I would not make function with such specific name provide additionally size info... I think it is perfectily fine to have many monitor commands that internally use same 'query-block' qemu command because this command is really profound)

Note that I didn't say add a new param/return - I said make it more multi-purpose with a subtly implied inference that perhaps a function name change and a different return value (or local/static structure) could work as well (and hence why I tried to combine things today to see what was possible).

...

...

...

typedef struct _qemuDomainHostdevPrivate qemuDomainHostdevPrivate; diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 3d0a120..7903b47 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1800,6 +1800,8 @@ int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon,

for (i = 0; i < virJSONValueArraySize(devices); i++) { virJSONValuePtr dev = virJSONValueArrayGet(devices, i); + virJSONValuePtr inserted; + virJSONValuePtr image; struct qemuDomainDiskInfo *info; const char *thisdev; const char *status; @@ -1854,6 +1856,21 @@ int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon, if (info->io_status < 0) goto cleanup; } + + if ((inserted = virJSONValueObjectGetObject(dev, "inserted"))) { + if (!(image = virJSONValueObjectGetObject(inserted, "image"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot read 'inserted/image' value")); + goto cleanup; + }

Other code that checks "inserted" and "image" doesn't fail - it just ignores. If there's only going to be one consumer, then I don't believe we want a failure such as this to affect other callers that may not care. That could mean having some sort of "marker" in the returned buffer that the fetch of "virtual-size" did occur (or just using not zero). It would be a shame to have some unexpected failures for a field that nothing else uses especially since the *GetBlockInfo is being used during process launch and reconnect (via qemuProcessRefreshDisks).

I doubt there can be 'inserted' without 'image', it doesn't make sense,

True, but I'm concerned that by adding an error path to existing code there's some 'strange' path or device that previously wasn't causing an error that now would be. It's the paranoia of this code. I think eblake has extensive knowledge of the QEMU side/layout.

...

inserted what?)) I guess qemuDomainGetStatsBlock ignores missing 'image' because it can afford it, while qemuMonitorJSONDiskNameLookup does not really ignores, if name is not found then function return error eventually. So if this is true, there should be no 'inserted' without 'image', then shame will go to qemu)

...

Futhermore, what if there's a "backing-image" for "this" particular path? Will that be important for the pull backups support? Without looking at patch 3 I would think so...

Not really. Backing chain has nothing to do with backup, there is just no difference from backup POV.

Maybe it's my novice/naive knowledge of backing chains, but if you're taking a backing and some data was in the backing-chain would it be lost or does the API you're calling handle that for you. Again, I don't have

Backup sees disk just like guest, block device nothing more, no any backing chain structure.

a lot of exposure to that code. I do know there's still upstream qemu work taking place. We really should be very careful about adding anything to libvirt before the upstream work is done. In particular any x-* command support.

AFAIK full pull backups as well as full/incremennal push backups are ready to use in qemu. It is a pity that other commands that helps organize pull backup process still has experimental status. One day they drop this x- and I doubt that things will changed very radically in comparison to what we have today. So if with the help of community this patch will be ironed out libvirt will get pull backups instantly after qemu be full ready. Nikolay

On 09/07/2016 05:24 AM, Nikolay Shirokovskiy wrote:

Special error code helps gracefully handle race conditions on blockjob cancelling. Consider for example pull backup. We want to stop it and in the process we cancel blockjob for some of the exported disks. But at the time this command reaches qemu blockjob fail for some reason and cancel result and result of stopping will be an error with quite unexpecte message - no such job (sort of). Instead we can detect this race and treat as correct cancelling and wait until fail event will arrive. Then we can report corect error message with some details from qemu probably.

Users of domainBlockJobAbort can be affected as -2 is new possible return code. Not sure if I should stick to the original behaviour in this case. --- src/qemu/qemu_monitor.c | 5 +++++ src/qemu/qemu_monitor_json.c | 11 +++++++---- 2 files changed, 12 insertions(+), 4 deletions(-)

I haven't looked forward to patch 3/3 yet, but perhaps it'd be better in the "caller" that cares to : if (qemu*() < 0) { virErrorPtr err = virGetLastError(); if (err && err->code == VIR_ERR_OPERATION_INVALID) { /* Do something specific */ } } Returning -2 alters virDomainBlockJobAbort expected return values and well that's probably not a good idea since we don't know if some caller has said (if virDomainBlockJobAbort() == -1) instead of <= -1 John

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 4171914..944856d 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3335,6 +3335,11 @@ qemuMonitorBlockStream(qemuMonitorPtr mon, }

+/* return: + * 0 in case of success + * -1 in case of general error + * -2 in case there is no such job + */ int qemuMonitorBlockJobCancel(qemuMonitorPtr mon, const char *device, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 7903b47..42b05c4 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4351,6 +4351,7 @@ qemuMonitorJSONBlockJobError(virJSONValuePtr reply, if (qemuMonitorJSONErrorIsClass(error, "DeviceNotActive")) { virReportError(VIR_ERR_OPERATION_INVALID, _("No active operation on device: %s"), device); + return -2; } else if (qemuMonitorJSONErrorIsClass(error, "DeviceInUse")) { virReportError(VIR_ERR_OPERATION_FAILED, _("Device %s in use"), device); @@ -4408,6 +4409,11 @@ qemuMonitorJSONBlockStream(qemuMonitorPtr mon, }

+/* return: + * 0 in case of success + * -1 in case of general error + * -2 in case there is no such job + */ int qemuMonitorJSONBlockJobCancel(qemuMonitorPtr mon, const char *device, @@ -4426,10 +4432,7 @@ qemuMonitorJSONBlockJobCancel(qemuMonitorPtr mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) goto cleanup;

- if (qemuMonitorJSONBlockJobError(reply, cmd_name, device) < 0) - goto cleanup; - - ret = 0; + ret = qemuMonitorJSONBlockJobError(reply, cmd_name, device);

cleanup: virJSONValueFree(cmd);

On 27.09.2016 01:04, John Ferlan wrote:

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!).

Ok, at least the patch can be some kind of candidate to be pushed upstream as soon as blockdev-del drops x- prefix.

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@virtuozzo.com> + * + * + * 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.

Totally agree.

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.

Well it is just a flag, just as in case of snapshots. As to coding conventions etc a lot of stuff is rooted in similar snapshot code and xml.

...

+} 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"

but there is VIR_FROM_DOMAIN_SNAPSHOT... and it is more pleasant for eyes.

...

# 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}

similar to snapshot_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

this mimics virsh-snapshot

...

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

alignment is good here, it is just tabs issue) looks like Makefile.am has ts=8 and this may differ from mail client settings. Anyway this file indentation is rather messy, mixing tabs and spaces...

...

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

this is fair

...

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@virtuozzo.com> + */ + +#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

Name 'tcp' is more appropriate indeed )) I would not use virStorageNetHostTransport and associated structures and xml becasue they have different semantics. In case of backups we have address client can connect to, in case of network disks we have address qemu as client can connect to.

FYI: also see qemuMonitorGraphicsAddressFamily

And this one has right semantics. I remember I evaluated it as reuse candidate. I thought it was awkward to have port in upper level element: <graphics type='vnc' port='5904' sharePolicy='allow-exclusive'> <listen type='address' address='1.2.3.4'/> </graphics> if we could declare it outdated and recommend to use new style... <graphics type='vnc' sharePolicy='allow-exclusive'> <listen type='address' address='1.2.3.4' port='5904' /> (autoport, tlsPort etc goes here too...) </graphics> Actually we already do this for address attribute: "The address attribute is duplicated as listen attribute in graphics element for backward compatibility. If both are provided they must be equal." So if you are ok with this, i would definetly reuse this element and associated structures.

...

+ "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?

Sorry, this is messy. I should give a different name to attribute that specified full/increment backup in the cover. The code is ok. This attribute has the same meaning as in case of snapshots or disks in domain xml.

...

+ 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?

I just decided not to hurry to declare supported backup options. file + qcow2(probably raw too) is all I have tested.

...

+ + 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...

to do or not to do) backup or not to backup this particular disk. The choice is not successful I as see it now, "backup" is better with choices default/off/incremental/full similar to what snapshots have.

...

+ 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

ouch, copy paste mistake

...

+ goto cleanup; + }

And your <bitmap> from the cover is missing here.

i was talking about future, incremental pull backups are not yet implemented in qemu. So why hurry introducing this element in xml.

...

+ + 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?

AFAIK there is not any yet for pull backups. The situation is in case of migration, it is not secure when moving state and disks. This is what Daniel is working on AFAIK.

...

+ 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?

At least to specify where temporary backup files should be.

...

+ + 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"?

why? element name has no _, to be aligned with snapshots.

...

+ 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?

looks like this name does not do much and again it is copy paste from snapshots, not sure why it has _.

...

+ 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?

But no one needs it now, just don't wanted to introduce dangling code...

...

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@virtuozzo.com> + */ + +#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@virtuozzo.com> + */ + +#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.

i'll check, but looks like bool is enough. There is 'migrating' by the way, I can name it 'backup_active' if you prefer.

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!

ok, backupdev_created

...

+ 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...

Of course, that is why this comment is here. Can we add the functionality first and then make appropriate refactoring? It is quite a different and probably profound task.

...

+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?

It is a pull backup. User don't really need this temporary file. It is keeped only in the process of backup as inverse delta to keep disk state at the moment of backup start. So there is no reason to rename. As to randomly generated names... Do we really need this? We can afford only one backup at a time, one backup delta per disk...

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.

You can not check space at all. The size of backup inverse delta heavily depends on guest activity. It can be 0, it can be full disk size.

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.

While this is true I thought this can be done in next patches which can address other issues like disk cleanups, handling daemon restart etc.

...

+ 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.

Well I have a copy-paste argument ) IOW snapshots use this already. It comes from b60af444 and here to handle special case when image paths differ only in suffix. Ok, to make some really bulletproof protection we need to make functions like virStorageFileBackendFileCreate to include O_EXCL on open.

...

+ 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...

At least we stay at O(n) complexity. '200 disks' is for real?

...

+ 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...

It is quite ubiquitous to use ! check for undefined/default in libvirt. However this is far from principal, so as you prefer

...

+ 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?

Just future plan ))

...

+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.

You mean track nbd server usage? To give better errors or smth?

...

+ 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...

I tried to address failure in code, so that flags are triggered after specific action in the loop, like 'blockdev'. I doubt that splitting the loop will make code more comprehensible. On cleanup you need to undo setup actions in reverse order starting from last successful one which means use 'i' counter after goto and this is more complicated than setup per disk flags in my opinion.

...

+ 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..

Well copy list just to drop 2 line check while structure is mainly the same...

...

+ + 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.

good catch

...

+ + 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?

Well, no. This function is used by migration code and there is no unix sockets support because migration is always between different hosts.

...

+ 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.

Ok, most of the code use different label instead of flag. IMHO flag is more simple to use, for example when you move a expression you don't need to accomodate label. However to keep the style I'll replace to endjob label.

...

+ + 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

While it is possible to move this check there I think it will blur the function semantics. It quite unexpectable to find this check in the function with such a name. The names says it only fill spaces in backup xml.

...

+ } + } + + /* 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?

nope, in case QUIESCE is not set frozen is -1

...

+ 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

Thank you for review! Nikolay

...

+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@virtuozzo.com> + */ + +#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@virtuozzo.com> + */ + +#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"

On 30.09.2016 01:02, John Ferlan wrote:

[...]

...

...

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!).

Ok, at least the patch can be some kind of candidate to be pushed upstream as soon as blockdev-del drops x- prefix.

Not as a single patch though - I have a feeling you're going to have a git branch committed to this for a while... Consider my recent response to your 1/3 patch w/r/t qemu's statement about the blockdev-add command...

I can split it to API/rpc/driver based stubs/qemu implementation as usual. I just thought all parts except qemu implementation is a kind of boilerplate and do not need much attention... Hmm, this can be an agrgument to split this into 2 parts - boilerplate/qemu implementation.

My *guess* is when 'blockdev-del' is officially supported - that's when libvirt can "assume" support for blockdev-add (since the x- was taken off of it - something that we can document when that happens).

[...]

...

...

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.

Well it is just a flag, just as in case of snapshots. As to coding conventions etc a lot of stuff is rooted in similar snapshot code and xml.

Something that just needs to be remembered - oh and documented in virsh.pod...

I'd delayed pod until we ready to push upstream, anything interesting will be in a cover letter like just right now but in a less formal form))

[...]

...

...

...

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"

but there is VIR_FROM_DOMAIN_SNAPSHOT... and it is more pleasant for eyes.

OK - now that I know SNAPSHOT is your basis... BTW: IIRC the snapshot code didn't make it all in one patch...

...

...

...

# 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}

similar to snapshot_conf.{c,h}

It's just a file name in the long run; however, what if in the future the storage driver added some sort of backup, then we'd have 'storage_backup_conf.c' to clearly describe that it was meant for storage. I think the domain_<function>_conf.c should be used... And yes, the same argument can be made for snapshot_conf.c - perhaps it should be renamed.

[...]

...

...

...

+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

Name 'tcp' is more appropriate indeed )) I would not use virStorageNetHostTransport and associated structures and xml becasue they have different semantics. In case of backups we have address client can connect to, in case of network disks we have address qemu as client can connect to.

Creating a specific name is fine - I was just pointing out the StorageNet for names to consider.

...

...

FYI: also see qemuMonitorGraphicsAddressFamily

And this one has right semantics. I remember I evaluated it as reuse candidate. I thought it was awkward to have port in upper level element:

<graphics type='vnc' port='5904' sharePolicy='allow-exclusive'> <listen type='address' address='1.2.3.4'/> </graphics>

if we could declare it outdated and recommend to use new style...

<graphics type='vnc' sharePolicy='allow-exclusive'> <listen type='address' address='1.2.3.4' port='5904' /> (autoport, tlsPort etc goes here too...) </graphics>

Actually we already do this for address attribute: "The address attribute is duplicated as listen attribute in graphics element for backward compatibility. If both are provided they must be equal."

So if you are ok with this, i would definetly reuse this element and associated structures.

Again - just pointed out different things to consider. My knowledge of blockdev-backup is miniscule (I think I spelled it right).

[...]

...

...

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?

AFAIK there is not any yet for pull backups. The situation is in case of migration, it is not secure when moving state and disks. This is what Daniel is working on AFAIK.

Just something that may have to be considered - although who knows. I've recently added LUKS support and now am being told about all those corner cases that weren't considered and where things didn't work quite right - so I'm just trying to consider various things as they come to mind. With any luck the end result will be better.

[...]

...

...

...

+ + if ((n = virXPathNodeSet("./disks/*", ctxt, &nodes)) < 0) + goto cleanup;

So the purpose of disks is what?

At least to specify where temporary backup files should be.

There's nothing specific in the <disks> xml - you could just list 'n' disks and use specific XML API's to get you a count of <disk> elements. Thus the <disks> just seemed superfluous.

This is mimicing snapshots too... Looks like <disks> is redundant yes, but for the sake of 'least surprise' principle I would keep it.

[...]

...

...

...

--- 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.

i'll check, but looks like bool is enough. There is 'migrating' by the way, I can name it 'backup_active' if you prefer.

It just looked funny to read...

...

...

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!

ok, backupdev_created

Sounds like you need a enum that can go through states "not desired", "desired", "selected", "in process", "succeeded", etc.

True, I had this feeling too to some extent. I'll look deeper into this opportunity in next version.

...

...

...

+ bool backupFailed; +

And this is the one that is the kicker... Once you have failed, you'd need to decide how to handle any others that were "in process" somehow or had been processed successfully. Error recovery could be a full time job - similar to error recovery for snapshot failures (internal, external, make my head spin)

I took simple approach. Stop operation finishes with the error in this case. One don't need complex recovery with backups because they are unobtrusive for user supplied disks. On any error you probably get some garbage in backup targets but user disks are totally untouched.

[...]

...

...

...

+ 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?

It is a pull backup. User don't really need this temporary file. It is keeped only in the process of backup as inverse delta to keep disk state at the moment of backup start. So there is no reason to rename.

As to randomly generated names... Do we really need this? We can afford only one backup at a time, one backup delta per disk...

I thought there was a whole naming thing with snapshots - not sure. Certainly synergies can be made.

...

...

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.

You can not check space at all. The size of backup inverse delta heavily depends on guest activity. It can be 0, it can be full disk size.

Ok - again something that having done backups before I'd have to consider... It's more of a general comment...

...

...

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.

While this is true I thought this can be done in next patches which can address other issues like disk cleanups, handling daemon restart etc.

If history has taught us anything it's that future patches sometimes aren't written... Leaving that task to someone else to pick up.

)) True. But hey my goal is stabilize backups API and its significant implementation part. We can hold it from pushing upstream as long as we wish and write missing parts meanwhile. However we deal with upstream and it is really a not-that-stable branch and it is up to user to use some feature or not if is is not complete in all aspects (reality: many start to try then use)

...

...

...

+ 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.

Well I have a copy-paste argument ) IOW snapshots use this already. It comes from b60af444 and here to handle special case when image paths differ only in suffix. Ok, to make some really bulletproof protection we need to make functions like virStorageFileBackendFileCreate to include O_EXCL on open.

Again - if snapshots is your basis then we should consider more code sharing options. Not sure how to accomplish that at this point, but that's the suggestion at this point in time.

[...]

...

...

...

+ /* 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...

At least we stay at O(n) complexity. '200 disks' is for real?

In a former company, 1000 was the test case. People uses guests for all different kinds of things. I don't have that many disks, but there is a numerical problem. Recently it was pointed out on list that going through ndisks in migration code should be done in one place and let's try to limit the number of times we need to run through the list.

Works great in my test environment of 1 or 2 disks, can't understand why it's failing for (name your favorite large company) with many more disks...

It is a test issue actually. Nothing prevents this code work well with large collection of disk from theoretical POV and it is impossible to predict how minizing iterations will influence on this aspect... But I'll give it a try, just not sure what are tradeoffs from number of lines/program structure complexity POV

[...]

...

...

...

+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.

You mean track nbd server usage? To give better errors or smth?

Like you have a reliance on guest agent for quiesce - you have a reliance on nbd server to do something...

I think this is where I went back and wrote you need to consider networked disks in your XML (the auth portion).

I have bz that's looking to add/allow TLS for NBD server starts...

...

...

...

+ 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...

I tried to address failure in code, so that flags are triggered after specific action in the loop, like 'blockdev'. I doubt that splitting the loop will make code more comprehensible. On cleanup you need to undo setup actions in reverse order starting from last successful one which means use 'i' counter after goto and this is more complicated than setup per disk flags in my opinion.

At this point the brain is already going to mush and the eyes are tired, but you get the point - there's something "not quite right" with number of times going through the loop.

[...]

...

...

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

Thank you for review!

Nikolay

Thank you for starting to think about this. It's a tricky and complex issue based upon code that isn't quite baked in QEMU yet. I think given more cycles in my day and perhaps smaller piles of patches eventually we'll hammer out some working code. I'm sure I'll learn a lot about blockdev during the next few months too...

Sounds great! Nikolay

On 05.10.2016 00:55, John Ferlan wrote:

On 09/30/2016 10:38 AM, Nikolay Shirokovskiy wrote:

...

On 30.09.2016 01:02, John Ferlan wrote:

...

[...]

...

...

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!).

Ok, at least the patch can be some kind of candidate to be pushed upstream as soon as blockdev-del drops x- prefix.

Not as a single patch though - I have a feeling you're going to have a git branch committed to this for a while... Consider my recent response to your 1/3 patch w/r/t qemu's statement about the blockdev-add command...

I can split it to API/rpc/driver based stubs/qemu implementation as usual. I just thought all parts except qemu implementation is a kind of boilerplate and do not need much attention... Hmm, this can be an agrgument to split this into 2 parts - boilerplate/qemu implementation.

See: http://libvirt.org/api_extension.html

I strongly suggest using gitk... Since you'll be making a change to libvirt_public.syms and remote_protocol.x... Make a "test" change to either one.. Then use gitk to "view" the previous changes to that file. You can right click on a previously added line to get a popup menu that allows you to "Show origin of this line"... Selecting that and you'll see the set the patches for that change... You'll probably see 7-15 patches in the same general area from the same author. I would look at a few... Get a feel for how others have had similar changes accepted.

If nothing else it gives you an idea of the files that need to change and the order of the changes so that you don't drop huge one patch bombs on the list.

All that said - I know from the recent review of my qemu_monitor_json series that pkrempa is already thinking about this area of the code... Besides as I hope I pointed out - don't do anything until qemu is ready for changes to be made.

But ... I thought it is agreed upon that we'll try to create backup patch in parallell with qemu finishing its part. Even if there will be changes in qemu or in libvirt seems they don't throw away most of the work. The way we find disks size or the way we pass parameters to blockdev-add are quite isolated parts of the patch... Nikolay

...

...

My *guess* is when 'blockdev-del' is officially supported - that's when libvirt can "assume" support for blockdev-add (since the x- was taken off of it - something that we can document when that happens).

[...]

...

...

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.

Well it is just a flag, just as in case of snapshots. As to coding conventions etc a lot of stuff is rooted in similar snapshot code and xml.

Something that just needs to be remembered - oh and documented in virsh.pod...

I'd delayed pod until we ready to push upstream, anything interesting will be in a cover letter like just right now but in a less formal form))

...

[...]

...

...

...

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"

but there is VIR_FROM_DOMAIN_SNAPSHOT... and it is more pleasant for eyes.

OK - now that I know SNAPSHOT is your basis... BTW: IIRC the snapshot code didn't make it all in one patch...

...

...

...

# 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}

similar to snapshot_conf.{c,h}

It's just a file name in the long run; however, what if in the future the storage driver added some sort of backup, then we'd have 'storage_backup_conf.c' to clearly describe that it was meant for storage. I think the domain_<function>_conf.c should be used... And yes, the same argument can be made for snapshot_conf.c - perhaps it should be renamed.

[...]

...

...

...

+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

Name 'tcp' is more appropriate indeed )) I would not use virStorageNetHostTransport and associated structures and xml becasue they have different semantics. In case of backups we have address client can connect to, in case of network disks we have address qemu as client can connect to.

Creating a specific name is fine - I was just pointing out the StorageNet for names to consider.

...

...

FYI: also see qemuMonitorGraphicsAddressFamily

And this one has right semantics. I remember I evaluated it as reuse candidate. I thought it was awkward to have port in upper level element:

<graphics type='vnc' port='5904' sharePolicy='allow-exclusive'> <listen type='address' address='1.2.3.4'/> </graphics>

if we could declare it outdated and recommend to use new style...

<graphics type='vnc' sharePolicy='allow-exclusive'> <listen type='address' address='1.2.3.4' port='5904' /> (autoport, tlsPort etc goes here too...) </graphics>

Actually we already do this for address attribute: "The address attribute is duplicated as listen attribute in graphics element for backward compatibility. If both are provided they must be equal."

So if you are ok with this, i would definetly reuse this element and associated structures.

Again - just pointed out different things to consider. My knowledge of blockdev-backup is miniscule (I think I spelled it right).

[...]

...

...

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?

AFAIK there is not any yet for pull backups. The situation is in case of migration, it is not secure when moving state and disks. This is what Daniel is working on AFAIK.

Just something that may have to be considered - although who knows. I've recently added LUKS support and now am being told about all those corner cases that weren't considered and where things didn't work quite right - so I'm just trying to consider various things as they come to mind. With any luck the end result will be better.

[...]

...

...

...

+ + if ((n = virXPathNodeSet("./disks/*", ctxt, &nodes)) < 0) + goto cleanup;

So the purpose of disks is what?

At least to specify where temporary backup files should be.

There's nothing specific in the <disks> xml - you could just list 'n' disks and use specific XML API's to get you a count of <disk> elements. Thus the <disks> just seemed superfluous.

This is mimicing snapshots too... Looks like <disks> is redundant yes, but for the sake of 'least surprise' principle I would keep it.

...

[...]

...

...

...

--- 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.

i'll check, but looks like bool is enough. There is 'migrating' by the way, I can name it 'backup_active' if you prefer.

It just looked funny to read...

...

...

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!

ok, backupdev_created

Sounds like you need a enum that can go through states "not desired", "desired", "selected", "in process", "succeeded", etc.

True, I had this feeling too to some extent. I'll look deeper into this opportunity in next version.

...

...

...

...

+ bool backupFailed; +

And this is the one that is the kicker... Once you have failed, you'd need to decide how to handle any others that were "in process" somehow or had been processed successfully. Error recovery could be a full time job - similar to error recovery for snapshot failures (internal, external, make my head spin)

I took simple approach. Stop operation finishes with the error in this case.

One don't need complex recovery with backups because they are unobtrusive for user supplied disks. On any error you probably get some garbage in backup targets but user disks are totally untouched.

...

[...]

...

...

...

+ 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?

It is a pull backup. User don't really need this temporary file. It is keeped only in the process of backup as inverse delta to keep disk state at the moment of backup start. So there is no reason to rename.

As to randomly generated names... Do we really need this? We can afford only one backup at a time, one backup delta per disk...

I thought there was a whole naming thing with snapshots - not sure. Certainly synergies can be made.

...

...

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.

You can not check space at all. The size of backup inverse delta heavily depends on guest activity. It can be 0, it can be full disk size.

Ok - again something that having done backups before I'd have to consider... It's more of a general comment...

...

...

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.

While this is true I thought this can be done in next patches which can address other issues like disk cleanups, handling daemon restart etc.

If history has taught us anything it's that future patches sometimes aren't written... Leaving that task to someone else to pick up.

)) True. But hey my goal is stabilize backups API and its significant implementation part. We can hold it from pushing upstream as long as we wish and write missing parts meanwhile. However we deal with upstream and it is really a not-that-stable branch and it is up to user to use some feature or not if is is not complete in all aspects (reality: many start to try then use)

...

...

...

...

+ 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.

Well I have a copy-paste argument ) IOW snapshots use this already. It comes from b60af444 and here to handle special case when image paths differ only in suffix. Ok, to make some really bulletproof protection we need to make functions like virStorageFileBackendFileCreate to include O_EXCL on open.

Again - if snapshots is your basis then we should consider more code sharing options. Not sure how to accomplish that at this point, but that's the suggestion at this point in time.

[...]

...

...

...

+ /* 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...

At least we stay at O(n) complexity. '200 disks' is for real?

In a former company, 1000 was the test case. People uses guests for all different kinds of things. I don't have that many disks, but there is a numerical problem. Recently it was pointed out on list that going through ndisks in migration code should be done in one place and let's try to limit the number of times we need to run through the list.

Works great in my test environment of 1 or 2 disks, can't understand why it's failing for (name your favorite large company) with many more disks...

It is a test issue actually. Nothing prevents this code work well with large collection of disk from theoretical POV and it is impossible to predict how minizing iterations will influence on this aspect... But I'll give it a try, just not sure what are tradeoffs from number of lines/program structure complexity POV

...

[...]

...

...

...

+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.

You mean track nbd server usage? To give better errors or smth?

Like you have a reliance on guest agent for quiesce - you have a reliance on nbd server to do something...

I think this is where I went back and wrote you need to consider networked disks in your XML (the auth portion).

I have bz that's looking to add/allow TLS for NBD server starts...

...

...

...

+ 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...

I tried to address failure in code, so that flags are triggered after specific action in the loop, like 'blockdev'. I doubt that splitting the loop will make code more comprehensible. On cleanup you need to undo setup actions in reverse order starting from last successful one which means use 'i' counter after goto and this is more complicated than setup per disk flags in my opinion.

At this point the brain is already going to mush and the eyes are tired, but you get the point - there's something "not quite right" with number of times going through the loop.

[...]

...

...

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

Thank you for review!

Nikolay

Thank you for starting to think about this. It's a tricky and complex issue based upon code that isn't quite baked in QEMU yet. I think given more cycles in my day and perhaps smaller piles of patches eventually we'll hammer out some working code. I'm sure I'll learn a lot about blockdev during the next few months too...

Sounds great!

Nikolay