10:45 a.m.
On 09/27/2011 08:39 AM, Michal Privoznik wrote:
This element says what to do with cdrom (or floppy) on migration.
Currently, only one attribute is supported: 'optional'. It accepts
'yes' and 'no' values. Setting a cdrom to be optional means, if
destination cannot access disk source, it simply gets free()'d/ejected.
This functionality is important for users, whose machines get buggy.
So they decide to save as much as possible and migrate the machine
even they won't be able to access (readonly) cdrom on destination.
---
docs/schemas/domaincommon.rng | 16 ++++++++
Missing docs/formatdomain.html.in, so I can't ack this. That said, I'll
still review...
src/conf/domain_conf.c | 85
++++++++++++++++++++++++++++++++++++++++-
src/conf/domain_conf.h | 16 ++++++++
src/libvirt_private.syms | 2 +
4 files changed, 118 insertions(+), 1 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index be98be0..1150297 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -630,6 +630,9 @@
<ref name="encryption"/>
</optional>
<optional>
+<ref name="migration"/>
+</optional>
+<optional>
This allows <migration> for all disk types. With a bit more RNG magic,
it might be possible to enforce that <migration> only appears for cdrom
and floppy. On the other hand, it might be possible to further extend
<migration> for how it interacts with snapshots and/or copy-on-read
behaviors (for example, a future patch might make it possible to request
that migration creates a qcow2 wrapper on the destination with the
source as its backing file, then after migration completes, perform a
block pull to break the remote link), so I'm okay with leaving
<migration> for all three disk types with future extensibility in mind.
+++ b/src/conf/domain_conf.c
@@ -560,6 +560,12 @@ VIR_ENUM_IMPL(virDomainNumatuneMemMode,
VIR_DOMAIN_NUMATUNE_MEM_LAST,
"preferred",
"interleave");
+VIR_ENUM_IMPL(virDomainDeviceMigrationOptional,
+ VIR_DOMAIN_DEVICE_MIGRATION_OPT_LAST,
+ "default",
+ "yes",
+ "no");
Are these the right values for the attribute? I see at least three
choices, not two:
1. require - destination must see the same cd as source
2. optional - if destination has same cd, use it, if not, eject
3. drop - on migration, destination will eject, even if it has same cd
Also, does this play well with cdroms that the host has locked? That
is, since there is a difference between eject (which fails if guest has
lock) and eject -f (which succeeds at ripping out the disk regardless of
the guest's current use of the disk), we may need to factor in some
description of whether we use force, or whether the migration fails if a
guest lock is in place, and so forth.
+
#define virDomainReportError(code, ...) \
virReportErrorHelper(VIR_FROM_DOMAIN, code, __FILE__, \
__FUNCTION__, __LINE__, __VA_ARGS__)
@@ -764,6 +770,7 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def)
VIR_FREE(def->dst);
VIR_FREE(def->driverName);
VIR_FREE(def->driverType);
+ VIR_FREE(def->migration);
Store def->migration by value, not by reference, and life gets simpler.
More on this later...
virStorageEncryptionFree(def->encryption);
virDomainDeviceInfoClear(&def->info);
@@ -2247,6 +2254,56 @@ cleanup:
}
+static int
+virDomainDeviceMigrationParseXML(xmlNodePtr node,
+ virDomainDiskDefPtr def)
+{
+ int ret = -1;
+ char *optional = NULL;
+ int i;
+
+ if (!node || !def) {
+ virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("invalid argument supplied"));
+ return -1;
+ }
+
+ if (VIR_ALLOC(def->migration)< 0) {
+ virReportOOMError();
+ return -1;
+ }
No need to alloc if you store by value.
@@ -9076,6 +9140,21 @@ virDomainLeaseDefFormat(virBufferPtr buf,
}
static int
+virDomainDeviceMigrationFormat(virBufferPtr buf,
+ virDomainDeviceMigrationInfoPtr mig)
+{
+ if (!buf)
+ return -1;
+
+ if (!mig)
+ return 0;
+
+ virBufferAsprintf(buf, "<migration optional='%s'/>\n",
+ virDomainDeviceMigrationOptionalTypeToString(mig->optional));
We shall see whether this needs a tweak once my v2 indentation patches
are in :)
+
+typedef struct _virDomainDeviceMigrationInfo virDomainDeviceMigrationInfo;
+typedef virDomainDeviceMigrationInfo *virDomainDeviceMigrationInfoPtr;
+struct _virDomainDeviceMigrationInfo {
+ int optional;
I guess it makes sense to break this into a separate struct, given my
argument above that we may extend <migration> further for other
migration-related aspects of how to handle disks.
+};
+
/* Stores the virtual disk configuration */
typedef struct _virDomainDiskDef virDomainDiskDef;
typedef virDomainDiskDef *virDomainDiskDefPtr;
@@ -286,6 +300,7 @@ struct _virDomainDiskDef {
unsigned int transient : 1;
virDomainDeviceInfo info;
virStorageEncryptionPtr encryption;
+ virDomainDeviceMigrationInfoPtr migration;
Rather than storing a virDomainDeviceMigrationInfoPtr that needs
migration, you can get away with virDomainDeviceMigrationInfo by value.
Allocating a pointer to a sub-struct is only needed when we want to be
able to easily pass around just the sub-struct, but I don't see that
happening here.
I think the biggest hurdle to clear is to get consensus on the xml
representation - I think your overall idea of dropping the disk source
of ejectable disks on migration makes sense, but am not convinced we
have the best xml representation for that notion yet.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org