noon
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(a)redhat.com>
> ---
> 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