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

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.