[libvirt] [PATCH 1/2] migration: Introduce <migration> element for cdrom and floppy

Eric Blake eblake at redhat.com
Tue Sep 27 15:45:20 UTC 2011


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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org




More information about the libvir-list mailing list