Re: [PATCH 1/6] internal: introduce macro helpers to reject exclusive flags

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

 



On Tue, Mar 24, 2015 at 04:40:17PM +0100, Peter Krempa wrote:
> On Fri, Mar 20, 2015 at 15:38:59 +0100, Pavel Hrdina wrote:
> > Inspired by commit 7e437ee7 that introduced similar macros for virsh
> > commands so we don't have to repeat the same code all over.
> > 
> > Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx>
> > ---
> >  src/internal.h | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 80 insertions(+)
> > 
> > diff --git a/src/internal.h b/src/internal.h
> > index 4d473af..5885f03 100644
> > --- a/src/internal.h
> > +++ b/src/internal.h
> > @@ -327,6 +327,86 @@
> >          }                                                               \
> >      } while (0)
> >  
> > +/* Macros to help dealing with mutually exclusive flags. */
> > +
> > +/**
> > + * VIR_EXCLUSIVE_FLAGS_EXPR_RET:
> > + *
> > + * @NAME1: String containing the name of the flag.
> > + * @EXPR1: Expression to validate the flag.
> > + * @NAME2: String containing the name of the flag.
> > + * @EXPR2: Expression to validate the flag.
> > + *
> > + * Reject mutually exclusive API flags.  Use the provided expression
> > + * to check the flags.
> > + *
> > + * This helper does an early return and therefore it has to be called
> > + * before anything that would require cleanup.
> > + */
> > +# define VIR_EXCLUSIVE_FLAGS_EXPR_RET(NAME1, EXPR1, NAME2, EXPR2)           \
> > +    if ((EXPR1) && (EXPR2)) {                                               \
> > +        virReportInvalidArg(ctl,                                            \
> > +                            _("Flags '%s' and '%s' are mutually exclusive"),\
> > +                            NAME1, NAME2);                                  \
> > +        return -1;                                                          \
> > +    }
> > +
> 
> While in virsh the above variant makes sense, because in some cases the
> variables have different names than the corresponding flags, in case of
> this code having it doesn't make sense.

You're right. I've only used the other macros.

> 
> 
> > +/**
> > + * VIR_EXCLUSIVE_FLAGS_VAR_RET:
> > + *
> > + * @FLAG1: First flag to be checked
> > + * @FLAG2: Second flag to be checked
> 
> A third argument @RET should be added here so that this can be used in
> places that also return NULL or any other possible variable.
> 
> In virsh the assumption is that the flags are checked at first and thus
> returning false makes sense, as every virsh command does that.

I'll add the @RET argument.

> 
> > + *
> > + * Reject mutually exclusive API flags.  The checked flags are compared
> > + * with flags variable.
> > + *
> > + * This helper does an early return and therefore it has to be called
> > + * before anything that would require cleanup.
> > + */
> > +# define VIR_EXCLUSIVE_FLAGS_RET(FLAG1, FLAG2)                              \
> > +    VIR_EXCLUSIVE_FLAGS_EXPR_RET(#FLAG1, flags & FLAG1, #FLAG2, flags & FLAG2)
> > +
> > +/**
> > + * VIR_EXCLUSIVE_FLAGS_EXPR_GOTO:
> > + *
> > + * @NAME1: String containing the name of the flag.
> > + * @EXPR1: Expression to validate the flag.
> > + * @NAME2: String containing the name of the flag.
> > + * @EXPR2: Expression to validate the flag.
> > + * @LABEL: label to jump to.
> > + *
> > + * Reject mutually exclusive API flags.  Use the provided expression
> > + * to check the flag.
> > + *
> > + * Returns nothing.  Jumps to a label if unsupported flags were
> > + * passed to it.
> > + */
> > +# define VIR_EXCLUSIVE_FLAGS_EXPR_GOTO(NAME1, EXPR1, NAME2, EXPR2, LABEL)   \
> > +    if ((EXPR1) && (EXPR2)) {                                               \
> > +        virReportInvalidArg(ctl,                                            \
> > +                            _("Flags '%s' and '%s' are mutually exclusive"),\
> > +                            NAME1, NAME2);                                  \
> > +        goto LABEL;                                                         \
> > +    }
> > +
> 
> Again, this function does not make sense in the library code.
> 
> > +/**
> > + * VIR_EXCLUSIVE_FLAGS_VAR_GOTO:
> > + *
> > + * @FLAG1: First flag to be checked.
> > + * @FLAG2: Second flag to be checked.
> > + * @LABEL: label to jump to.
> > + *
> > + * Reject mutually exclusive API flags.  The checked flags are compared
> > + * with flags variable.
> > + *
> > + * Returns nothing.  Jumps to a label if unsupported flags were
> > + * passed to it.
> > + */
> > +# define VIR_EXCLUSIVE_FLAGS_GOTO(FLAG1, FLAG2, LABEL)                      \
> > +    VIR_EXCLUSIVE_FLAGS_EXPR_GOTO(#FLAG1, flags & FLAG1,                    \
> > +                                  #FLAG2, flags & FLAG2, LABEL)
> > +
> > +
> >  # define virCheckNonNullArgReturn(argname, retval)  \
> >      do {                                            \
> >          if (argname == NULL) {                      \
> 
> While this is okay.
> 
> Peter

Thanks for review, I'll send v2.

Pavel

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]