On Fri, Jun 03, 2011 at 08:36:26AM -0600, Eric Blake wrote:
> 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.
Thanks for the info.
>
> 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.
OK.
>
> 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,
OK.
>
>
> > /* 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.
0.9.2 is released at June 6 (+0800)
>
> > -/* 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.
OK.
>
> > -/* 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).
OK.
>
> > -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.
OK.
--
Thanks,
Hu Tao
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list