Re: [RFC v14-rc2][PATCH 24/29] c/r: Add CR_COPY() macro (v4)

["Serge E. Hallyn" <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]

Quoting Oren Laadan (orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org):
> From: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> 
> As suggested by Dave[1], this provides us a way to make the copy-in and
> copy-out processes symmetric.  CR_COPY_ARRAY() provides us a way to do
> the same thing but for arrays.  It's not critical, but it helps us unify
> the checkpoint and restart paths for some things.
> 
> Changelog:
>     Mar 04:
>             . Removed semicolons
>             . Added build-time check for __must_be_array in CR_COPY_ARRAY
>     Feb 27:
>             . Changed CR_COPY() to use assignment, eliminating the need
>               for the CR_COPY_BIT() macro
>             . Add CR_COPY_ARRAY() macro to help copying register arrays,
>               etc
>             . Move the macro definitions inside the CR #ifdef
>     Feb 25:
>             . Changed WARN_ON() to BUILD_BUG_ON()
> 
> Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>
> 
> 1: https://lists.linux-foundation.org/pipermail/containers/2009-February/015821.html (all the way at the bottom)
> ---
>  include/linux/checkpoint.h |   26 ++++++++++++++++++++++++++
>  1 files changed, 26 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/checkpoint.h b/include/linux/checkpoint.h
> index 031e414..59ec563 100644
> --- a/include/linux/checkpoint.h
> +++ b/include/linux/checkpoint.h
> @@ -120,6 +120,32 @@ extern int cr_read_mm(struct cr_ctx *ctx);
>  extern int cr_read_fd_table(struct cr_ctx *ctx);
>  extern int cr_read_file(struct cr_ctx *ctx, int objref);
> 
> +/* useful macros to copy fields and buffers to/from cr_hdr_xxx structures */
> +#define CR_CPT 1
> +#define CR_RST 2
> +
> +#define CR_COPY(op, SAVE, LIVE)				        \
> +	do {							\
> +		if (op == CR_CPT)				\
> +			SAVE = LIVE;				\
> +		else						\
> +			LIVE = SAVE;				\
> +	} while (0)

Yes the above (SAVE+LIVE) is so much more useful to future coders,
thanks.

> +
> +/*
> + * Copy @count items from @LIVE to @SAVE if op is CR_CPT (otherwise,
> + * copy in the reverse direction)
> + */
> +#define CR_COPY_ARRAY(op, SAVE, LIVE, count)				\
> +	do {								\
> +		BUILD_BUG_ON(sizeof(*SAVE) != sizeof(*LIVE));		\
> +		if (op == CR_CPT)					\
> +			memcpy(SAVE, LIVE, count * sizeof(*SAVE));	\
> +		else							\
> +			memcpy(LIVE, SAVE, count * sizeof(*SAVE));	\
> +	} while (__must_be_array(SAVE) && __must_be_array(LIVE) && 0)

It doesn't really matter I guess, but I'd prefer to see:

#define CR_COPY_ARRAY(op, SAVE, LIVE, count)				\
	do {								\
		__must_be_array(SAVE);					\
		__must_be_array(LIVE);					\
		BUILD_BUG_ON(sizeof(*SAVE) != sizeof(*LIVE));		\
		if (op == CR_CPT)					\
			memcpy(SAVE, LIVE, count * sizeof(*SAVE));	\
		else							\
			memcpy(LIVE, SAVE, count * sizeof(*SAVE));	\
	} while (0)

Putting the __must_be_array()s inside the condition seems really weird.


> +
> +
>  #define cr_debug(fmt, args...)  \
>  	pr_debug("[%d:c/r:%s] " fmt, task_pid_vnr(current), __func__, ## args)
> 
> -- 
> 1.5.4.3