[libvirt] [PATCH v2 1/3] unify PARAM_CURRENT/PARAM_LIVE/PARAM_CONFIG flags

[Eric Blake]

On 06/02/2011 08:15 PM, Hu Tao wrote:
> This patch unify the PARAM_CURRENT/PARAM_LIVE/PARAM_CONFIG flags as
> Daniel Veillard suggested.

I concur with danpb's NACK.  See how I kept existing enum names while
adding a new consolidation enum in commit 824dcaf, as well as how I sunk
deprecated enum names to the bottom of the file in commig a9b3a78 as
well as documenting versions so a user knows whether to use the new or
the old name depending on how old of a libvirt they are targetting.  You
have to take the same approach for this patch to have any chance of
getting approved.

However, there is one exception - anything that did not exist in 0.9.1
is fair game, if we do it now.

>  /**
> + * virDomainParamFlags:
> + *
> + * common flags for commands that support --current, --live, --config
> + * and --force parameters
> + */
> +
> +typedef enum {
> +    VIR_DOMAIN_PARAM_CURRENT = 0,              /* affect current domain state */
> +    VIR_DOMAIN_PARAM_LIVE    = (1 << 0),       /* affect active domain */
> +    VIR_DOMAIN_PARAM_CONFIG  = (1 << 1),       /* affect next boot */
> +    VIR_DOMAIN_PARAM_FORCE = (1 << 2),         /* Forcibly modify device
> +                                                 (ex. force eject a cdrom) */
> +    VIR_DOMAIN_PARAM_MAXIMUM = (1 << 3),       /* affect Max rather than current */
> +} virDomainParamFlags;

If we introduce a new enum, we should _only_ list the three values
_CURRENT, _LIVE, and _CONFIG.  Leave other function-specific flags to
function-specific enums.  Furthermore, you _can't_ change values - an
older client that thinks that _MAXIMUM is 1<<2 calling a newer server
that thinks _MAXIMUM is 1<<3 is a recipe for disaster.

Also, I wonder if the names might be better represented as:

VIR_DOMAIN_AFFECT_CURRENT = 0,
VIR_DOMAIN_AFFECT_LIVE = 1<<1,
VIR_DOMAIN_AFFECT_CONFIG = 1<<2,


>  /* Management of scheduler parameters */
>  
> -typedef enum {
> -    VIR_DOMAIN_SCHEDPARAM_CURRENT = 0,        /* affect current domain state */
> -    VIR_DOMAIN_SCHEDPARAM_LIVE    = (1 << 0), /* Affect active domain */
> -    VIR_DOMAIN_SCHEDPARAM_CONFIG  = (1 << 1), /* Affect next boot */
> -} virDomainSchedParameterFlags;
> -

_IF_ we hurry, we can delete this enum prior to 0.9.2, since this
particular enum has not been released yet.  But if 0.9.2 is released, we
are stuck with these enum names.

> -/* flags for setting memory parameters */
> -typedef enum {
> -    VIR_DOMAIN_MEMORY_PARAM_CURRENT = 0,        /* affect current domain state */
> -    VIR_DOMAIN_MEMORY_PARAM_LIVE    = (1 << 0), /* affect active domain */
> -    VIR_DOMAIN_MEMORY_PARAM_CONFIG  = (1 << 1)  /* affect next boot */
> -} virMemoryParamFlags;

Likewise a set of unreleased enum names.

> -/* Memory size modification flags. */
> -typedef enum {
> -    VIR_DOMAIN_MEM_CURRENT = 0,        /* affect current domain state */
> -    VIR_DOMAIN_MEM_LIVE    = (1 << 0), /* affect active domain */
> -    VIR_DOMAIN_MEM_CONFIG  = (1 << 1), /* affect next boot */
> -    VIR_DOMAIN_MEM_MAXIMUM = (1 << 2), /* affect Max rather than current */
> -} virDomainMemoryModFlags;

Must be kept; these existed in 0.9.1.  Furthermore, this is a case where
VIR_DOMAIN_MEM_MAXIMUM cannot change in value.

> -/* Flags for controlling virtual CPU hot-plugging.  */
> -typedef enum {
> -    /* Must choose at least one of these two bits; SetVcpus can choose both */
> -    VIR_DOMAIN_VCPU_LIVE    = (1 << 0), /* Affect active domain */
> -    VIR_DOMAIN_VCPU_CONFIG  = (1 << 1), /* Affect next boot */
> -
> -    /* Additional flags to be bit-wise OR'd in */
> -    VIR_DOMAIN_VCPU_MAXIMUM = (1 << 2), /* Max rather than current count */
> -} virDomainVcpuFlags;

Must be kept; these existed in 0.9.1.  VIR_DOMAIN_VCPU_MAXIMUM cannot
change in value.  However, we _could_ add VIR_DOMAIN_VCPU_CURRENT == 0
(although that should be a separate patch, and preferably post-0.9.2).

> -typedef enum {
> -
> -   VIR_DOMAIN_DEVICE_MODIFY_CURRENT = 0, /* Modify device allocation based on current domain state */
> -   VIR_DOMAIN_DEVICE_MODIFY_LIVE = (1 << 0), /* Modify live device allocation */
> -   VIR_DOMAIN_DEVICE_MODIFY_CONFIG = (1 << 1), /* Modify persisted device allocation */
> -   VIR_DOMAIN_DEVICE_MODIFY_FORCE = (1 << 2), /* Forcibly modify device
> -                                                 (ex. force eject a cdrom) */
> -} virDomainDeviceModifyFlags;

Must be kept; these existed in 0.9.1.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110603/784649db/attachment-0001.sig>