[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]