Show error messages when failing early in the init process

Submitted by Nicolas Viennot on Sept. 6, 2019, 7:26 p.m.

Details

Message ID 1703fb9231fa494f922275f7f632093f@EXMBDFT10.ad.twosigma.com
State New
Series "Show error messages when failing early in the init process"
Headers show

Commit Message

Nicolas Viennot Sept. 6, 2019, 7:26 p.m.
* Shows an error message when failing to open images directory
* Fixes chdir() error message logging. Logging is not initialized yet
  making pr_perror() unavailable.

Signed-off-by: Nicolas Viennot <nviennot@twosigma.com>
---
 criu/cr-service.c | 2 +-
 criu/crtools.c    | 8 ++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/cr-service.c b/criu/cr-service.c
index 0938db02..5303eb77 100644
--- a/criu/cr-service.c
+++ b/criu/cr-service.c
@@ -336,7 +336,7 @@  static int setup_opts_from_req(int sk, CriuOpts *req)
 		SET_CHAR_OPTS(img_parent, req->parent_img);
 
 	if (open_image_dir(images_dir_path) < 0) {
-		pr_perror("Can't open images directory");
+		pr_perror("Can't open images directory %s", images_dir_path);
 		goto err;
 	}
 
diff --git a/criu/crtools.c b/criu/crtools.c
index 97a6d6d6..ab08c894 100644
--- a/criu/crtools.c
+++ b/criu/crtools.c
@@ -147,8 +147,11 @@  int main(int argc, char *argv[], char *envp[])
 	/* We must not open imgs dir, if service is called */
 	if (strcmp(argv[optind], "service")) {
 		ret = open_image_dir(opts.imgs_dir);
-		if (ret < 0)
+		if (ret < 0) {
+			pr_msg("Error: Can't open images directory %s: %s\n",
+			       opts.imgs_dir, strerror(errno));
 			return 1;
+		}
 	}
 
 	/*
@@ -162,7 +165,8 @@  int main(int argc, char *argv[], char *envp[])
 		pr_warn("Stopped and detached shell job will get SIGHUP from OS.\n");
 
 	if (chdir(opts.work_dir)) {
-		pr_perror("Can't change directory to %s", opts.work_dir);
+		pr_msg("Error: Can't change directory to %s: %s\n",
+		       opts.work_dir, strerror(errno));
 		return 1;
 	}
 

Comments

Andrei Vagin Sept. 11, 2019, 11:46 a.m.
On Fri, Sep 06, 2019 at 07:26:04PM +0000, Nicolas Viennot wrote:
> * Shows an error message when failing to open images directory
> * Fixes chdir() error message logging. Logging is not initialized yet
>   making pr_perror() unavailable.
> 
> Signed-off-by: Nicolas Viennot <nviennot@twosigma.com>
> ---
>  criu/cr-service.c | 2 +-
>  criu/crtools.c    | 8 ++++++--
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/criu/cr-service.c b/criu/cr-service.c
> index 0938db02..5303eb77 100644
> --- a/criu/cr-service.c
> +++ b/criu/cr-service.c
> @@ -336,7 +336,7 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
>  		SET_CHAR_OPTS(img_parent, req->parent_img);
>  
>  	if (open_image_dir(images_dir_path) < 0) {
> -		pr_perror("Can't open images directory");
> +		pr_perror("Can't open images directory %s", images_dir_path);
>  		goto err;
>  	}
>  
> diff --git a/criu/crtools.c b/criu/crtools.c
> index 97a6d6d6..ab08c894 100644
> --- a/criu/crtools.c
> +++ b/criu/crtools.c
> @@ -147,8 +147,11 @@ int main(int argc, char *argv[], char *envp[])
>  	/* We must not open imgs dir, if service is called */
>  	if (strcmp(argv[optind], "service")) {
>  		ret = open_image_dir(opts.imgs_dir);
> -		if (ret < 0)
> +		if (ret < 0) {
> +			pr_msg("Error: Can't open images directory %s: %s\n",
> +			       opts.imgs_dir, strerror(errno));

This doesn't look right.. We can't write errors to stdout. We need to
think more what we can do here. If we can't write into a log file, we
probably have to write errors to stderr.

>  			return 1;
> +		}
>  	}
>  
>  	/*
> @@ -162,7 +165,8 @@ int main(int argc, char *argv[], char *envp[])
>  		pr_warn("Stopped and detached shell job will get SIGHUP from OS.\n");
>  
>  	if (chdir(opts.work_dir)) {
> -		pr_perror("Can't change directory to %s", opts.work_dir);
> +		pr_msg("Error: Can't change directory to %s: %s\n",
> +		       opts.work_dir, strerror(errno));
>  		return 1;
>  	}
>  
> -- 
> 2.19.1
> 
> 
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
Dmitry Safonov Sept. 11, 2019, 11:46 a.m.
Hi Nicolas,


On Sat, 7 Sep 2019 at 07:57, Nicolas Viennot
<Nicolas.Viennot@twosigma.com> wrote:
[..]
> @@ -162,7 +165,8 @@ int main(int argc, char *argv[], char *envp[])
>                 pr_warn("Stopped and detached shell job will get SIGHUP from OS.\n");
>
>         if (chdir(opts.work_dir)) {
> -               pr_perror("Can't change directory to %s", opts.work_dir);
> +               pr_msg("Error: Can't change directory to %s: %s\n",
> +                      opts.work_dir, strerror(errno));
>                 return 1;
>         }

Please, separate this part from the rest of the patch.
This doesn't look like a proper fix.

Also, I believe it should go into early buffer.
+Cc: Adrian

The reason you don't see the message is probably because criu exits
earlier than flushes
that early print buffer. I would think that the better fix would be to
flush the early buffer in
a function with __attribute__((destructor))..

Thanks,
             Dmitry
Nicolas Viennot Sept. 19, 2019, 5:34 p.m.
Hello,

The commit introducing early log flush (c2cf4cfdc) resolved this issue.

Nico

-----Original Message-----
From: Dmitry Safonov <0x7f454c46@gmail.com> 
Sent: Wednesday, September 11, 2019 7:47 AM
To: Nicolas Viennot <Nicolas.Viennot@twosigma.com>; Adrian Reber <adrian@lisas.de>
Cc: criu@openvz.org
Subject: Re: [CRIU] [PATCH] Show error messages when failing early in the init process

Hi Nicolas,


On Sat, 7 Sep 2019 at 07:57, Nicolas Viennot <Nicolas.Viennot@twosigma.com> wrote:
[..]
> @@ -162,7 +165,8 @@ int main(int argc, char *argv[], char *envp[])
>                 pr_warn("Stopped and detached shell job will get 
> SIGHUP from OS.\n");
>
>         if (chdir(opts.work_dir)) {
> -               pr_perror("Can't change directory to %s", opts.work_dir);
> +               pr_msg("Error: Can't change directory to %s: %s\n",
> +                      opts.work_dir, strerror(errno));
>                 return 1;
>         }

Please, separate this part from the rest of the patch.
This doesn't look like a proper fix.

Also, I believe it should go into early buffer.
+Cc: Adrian

The reason you don't see the message is probably because criu exits earlier than flushes that early print buffer. I would think that the better fix would be to flush the early buffer in a function with __attribute__((destructor))..

Thanks,
             Dmitry