[CRIU] Re: [PATCH v2] parasite-syscall: some cleanup

[Pavel Emelyanov]

On 02/21/2012 08:19 PM, Kinsbursky Stanislav wrote:
> v2: command is set prior to parasite_execute() call now
> 
> Removed redundant on-stack variables typeof parasite_status_t and memcpy
> between them and parasite_ctl->addr_args object. Ando a lot of cleanup done
> around.
> 
> Signed-off-by: Stanislav Kinsbursky <skinsbursky at openvz.org>
> 
> ---
>  cr-dump.c                  |    2 +
>  include/parasite-syscall.h |    2 +
>  parasite-syscall.c         |   84 +++++++++++++++++++++-----------------------
>  3 files changed, 42 insertions(+), 46 deletions(-)
> 
> diff --git a/parasite-syscall.c b/parasite-syscall.c
> index 02a50fa..414ae0f 100644
> --- a/parasite-syscall.c
> +++ b/parasite-syscall.c
> @@ -36,6 +36,10 @@ static const char code_syscall[] = {0x0f, 0x05, 0xcc, 0xcc,
>  #define code_syscall_size	(round_up(sizeof(code_syscall), sizeof(long)))
>  #define parasite_size		(round_up(sizeof(parasite_blob), sizeof(long)))
>  
> +#define PARASITE_SET_CMD(ctl, cmd) {	\
> +	*(long *)ctl->addr_cmd = cmd;	\

Isn't it nicer to declare the ctl->addr_cmd as long * from the very beginning
and just write
   ctl->addr_cmd = cmd 
in the code?

> +}
> +
>  static int can_run_syscall(unsigned long ip, unsigned long start, unsigned long end)
>  {
>  	return ip >= start && ip < (end - code_syscall_size);
>  static int parasite_set_logfd(struct parasite_ctl *ctl, pid_t pid)
>  {
> -	parasite_status_t args = { };
>  	int ret;
>  
>  	ret = parasite_send_fd(ctl, get_logfd());
>  	if (ret)
>  		return ret;
>  
> -	ret = parasite_execute(PARASITE_CMD_SET_LOGFD, ctl, &args, sizeof(args));
> +	PARASITE_SET_CMD(ctl, PARASITE_CMD_SET_LOGFD);
> +	ret = parasite_execute(ctl);

ctl->args here contain some trash from the previous cmd execution. Who's about
to init it properly?

>  	if (ret < 0)
>  		return ret;
>  
> @@ -414,11 +411,10 @@ int parasite_dump_itimers_seized(struct parasite_ctl *ctl, struct cr_fdset *cr_f
>  				 CR_FD_ITIMERS, ctl, cr_fdset);
>  }
>  
> -int parasite_dump_misc_seized(struct parasite_ctl *ctl, struct parasite_dump_misc *misc)
> +int parasite_dump_misc_seized(struct parasite_ctl *ctl)
>  {
> -	return parasite_execute(PARASITE_CMD_DUMP_MISC, ctl,
> -				(parasite_status_t *)misc,
> -				sizeof(struct parasite_dump_misc));
> +	PARASITE_SET_CMD(ctl, PARASITE_CMD_DUMP_MISC);
> +	return parasite_execute(ctl);

I don't understand how the caller's data pointed formerly by misc are initialized.

>  }
>  
>  /*