[CRIU] Re: on servie fd handling code

[Pavel Emelyanov]

On 09/06/2012 12:12 PM, Cyrill Gorcunov wrote:
> Hi guys, would the patch below worth to have? What do you think?

3 thoughts inline.

> diff --git a/Makefile b/Makefile
> index c5ee775..aea0a8e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -42,6 +42,7 @@ OBJS		+= inotify.o
>  OBJS		+= signalfd.o
>  OBJS		+= pstree.o
>  OBJS		+= protobuf.o
> +OBJS		+= service-fd.o
>  
>  PROTOBUF-LIB	:= protobuf/protobuf-lib.o
>  
> diff --git a/crtools.c b/crtools.c
> index 901a6c7..1a5eb6d 100644
> --- a/crtools.c
> +++ b/crtools.c
> @@ -73,6 +73,9 @@ int main(int argc, char *argv[])
>  	opts.final_state = TASK_DEAD;
>  	INIT_LIST_HEAD(&opts.veth_pairs);
>  
> +	if (init_service_fd())
> +		return -1;
> +
>  	while (1) {
>  		static struct option long_opts[] = {
>  			{ "tree", required_argument, 0, 't' },
> diff --git a/include/crtools.h b/include/crtools.h
> index 8ad8ce8..a7ce246 100644
> --- a/include/crtools.h
> +++ b/include/crtools.h
> @@ -98,16 +98,6 @@ struct cr_options {
>  
>  extern struct cr_options opts;
>  
> -enum {
> -	LOG_FD_OFF = 1,
> -	LOG_DIR_FD_OFF,
> -	IMG_FD_OFF,
> -	SELF_EXE_FD_OFF,
> -	PROC_FD_OFF,
> -};
> -
> -int get_service_fd(int type);
> -
>  /* file descriptors template */
>  struct cr_fd_desc_tmpl {
>  	const char	*fmt;			/* format for the name */
> diff --git a/include/service-fd.h b/include/service-fd.h
> new file mode 100644
> index 0000000..749886e
> --- /dev/null
> +++ b/include/service-fd.h
> @@ -0,0 +1,18 @@
> +#ifndef SERVICE_FD_H__
> +#define SERVICE_FD_H__
> +
> +enum {
> +	LOG_FD_OFF	= 1,
> +	LOG_DIR_FD_OFF,
> +	IMG_FD_OFF,
> +	SELF_EXE_FD_OFF,
> +	PROC_FD_OFF,
> +	CTL_TTY_OFF,
> +
> +	SERVICE_FD_OFF__MAX,
> +};
> +
> +extern int init_service_fd(void);
> +extern int get_service_fd(int type);

Need the is_service_fd() call.

> +
> +#endif /* SERVICE_FD_H__ */
> diff --git a/include/util.h b/include/util.h
> index 1daf451..eaa3baf 100644
> --- a/include/util.h
> +++ b/include/util.h
> @@ -16,6 +16,8 @@
>  #include "types.h"
>  #include "log.h"
>  
> +#include "service-fd.h"
> +
>  #include "../protobuf/vma.pb-c.h"
>  
>  #define PREF_SHIFT_OP(pref, op, size)	((size) op (pref ##BYTES_SHIFT))
> diff --git a/service-fd.c b/service-fd.c
> new file mode 100644
> index 0000000..e0d2471
> --- /dev/null
> +++ b/service-fd.c
> @@ -0,0 +1,36 @@
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <limits.h>
> +
> +#include <sys/time.h>
> +#include <sys/resource.h>
> +
> +#include <fcntl.h>
> +
> +#include "compiler.h"
> +#include "types.h"
> +#include "log.h"
> +
> +#include "service-fd.h"
> +
> +static struct rlimit rlimit;

Don't cache the whole struct. The rlim_cur value is enough.

> +
> +int init_service_fd(void)
> +{
> +	/*
> +	 * Service FDs are thouse that most likely won't
> +	 * conflict with any 'real-life' ones
> +	 */
> +	if (getrlimit(RLIMIT_NOFILE, &rlimit)) {
> +		pr_perror("Can't get rlimit");
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +int get_service_fd(int type)
> +{
> +	return rlimit.rlim_cur - type;
> +}
> diff --git a/util.c b/util.c
> index 1fad14c..6f6cf6d 100644
> --- a/util.c
> +++ b/util.c
> @@ -224,23 +224,6 @@ int do_open_proc(pid_t pid, int flags, const char *fmt, ...)
>  	return openat(dirfd, path, flags);
>  }
>  
> -int get_service_fd(int type)
> -{
> -	struct rlimit rlimit;
> -
> -	/*
> -	 * Service FDs are thouse that most likely won't
> -	 * conflict with any 'real-life' ones
> -	 */
> -
> -	if (getrlimit(RLIMIT_NOFILE, &rlimit)) {
> -		pr_perror("Can't get rlimit");
> -		return -1;
> -	}
> -
> -	return rlimit.rlim_cur - type;
> -}

Leave the whole thing in util.c

> -
>  int copy_file(int fd_in, int fd_out, size_t bytes)
>  {
>  	ssize_t written = 0;
> --