[PATCHv2,1/4] mount: Add error messages

Submitted by Radostin Stoyanov on Nov. 17, 2019, 7:28 a.m.

Details

Message ID 20191117072837.10450-1-rstoyanov1@gmail.com
State New
Series "Series without cover letter"
Headers show

Commit Message

Radostin Stoyanov Nov. 17, 2019, 7:28 a.m.
Suggested-by: Andrei Vagin <avagin@gmail.com>
Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com>
---
 criu/mount.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/mount.c b/criu/mount.c
index 974af6eb2..301ba5d09 100644
--- a/criu/mount.c
+++ b/criu/mount.c
@@ -1536,6 +1536,7 @@  static __maybe_unused int mount_cr_time_mount(struct ns_id *ns, unsigned int *s_
 
 	ret = mount(source, target, type, 0, NULL);
 	if (ret < 0) {
+		pr_perror("Unable to mount %s %s", source, target);
 		exit_code = -errno;
 		goto restore_ns;
 	} else {
@@ -2004,7 +2005,10 @@  static int fetch_rt_stat(struct mount_info *m, const char *where)
 static int do_simple_mount(struct mount_info *mi, const char *src, const
 			   char *fstype, unsigned long mountflags)
 {
-	return mount(src, mi->mountpoint, fstype, mountflags, mi->options);
+	int ret = mount(src, mi->mountpoint, fstype, mountflags, mi->options);
+	if (ret)
+		pr_perror("Unable to mount %s %s (id=%d)", src, mi->mountpoint, mi->mnt_id);
+	return ret;
 }
 
 static char *mnt_fsname(struct mount_info *mi)
@@ -2491,8 +2495,11 @@  static int do_mount_one(struct mount_info *mi)
 		}
 
 		/* do_mount_root() is called from populate_mnt_ns() */
-		if (mount(opts.root, mi->mountpoint, NULL, MS_BIND | MS_REC, NULL))
+		if (mount(opts.root, mi->mountpoint, NULL, MS_BIND | MS_REC, NULL)) {
+			pr_perror("Unable to mount %s %s (id=%d)", opts.root, mi->mountpoint, mi->mnt_id);
 			return -1;
+		}
+
 		if (do_mount_root(mi))
 			return -1;
 		mi->mounted = true;

Comments

Pavel Tikhomirov Nov. 17, 2019, 9:37 a.m.
Haven't you lost the "Unable to remount" hunk from v1? And maybe you can 
write "v2:..." comments so it would be easier to understand what's changed.

On 11/17/19 10:28 AM, Radostin Stoyanov wrote:
> Suggested-by: Andrei Vagin <avagin@gmail.com>
> Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com>
> ---
>   criu/mount.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/criu/mount.c b/criu/mount.c
> index 974af6eb2..301ba5d09 100644
> --- a/criu/mount.c
> +++ b/criu/mount.c
> @@ -1536,6 +1536,7 @@ static __maybe_unused int mount_cr_time_mount(struct ns_id *ns, unsigned int *s_
>   
>   	ret = mount(source, target, type, 0, NULL);
>   	if (ret < 0) {
> +		pr_perror("Unable to mount %s %s", source, target);
>   		exit_code = -errno;
>   		goto restore_ns;
>   	} else {
> @@ -2004,7 +2005,10 @@ static int fetch_rt_stat(struct mount_info *m, const char *where)
>   static int do_simple_mount(struct mount_info *mi, const char *src, const
>   			   char *fstype, unsigned long mountflags)
>   {
> -	return mount(src, mi->mountpoint, fstype, mountflags, mi->options);
> +	int ret = mount(src, mi->mountpoint, fstype, mountflags, mi->options);
> +	if (ret)
> +		pr_perror("Unable to mount %s %s (id=%d)", src, mi->mountpoint, mi->mnt_id);
> +	return ret;
>   }
>   
>   static char *mnt_fsname(struct mount_info *mi)
> @@ -2491,8 +2495,11 @@ static int do_mount_one(struct mount_info *mi)
>   		}
>   
>   		/* do_mount_root() is called from populate_mnt_ns() */
> -		if (mount(opts.root, mi->mountpoint, NULL, MS_BIND | MS_REC, NULL))
> +		if (mount(opts.root, mi->mountpoint, NULL, MS_BIND | MS_REC, NULL)) {
> +			pr_perror("Unable to mount %s %s (id=%d)", opts.root, mi->mountpoint, mi->mnt_id);
>   			return -1;
> +		}
> +
>   		if (do_mount_root(mi))
>   			return -1;
>   		mi->mounted = true;
>
Radostin Stoyanov Nov. 17, 2019, 10:29 a.m.
On 17/11/2019 09:37, Pavel Tikhomirov wrote:
> Haven't you lost the "Unable to remount" hunk from v1? And maybe you can
> write "v2:..." comments so it would be easier to understand what's changed.
I'm sorry for the confusion. The same line was also being changed in the 
following patch, so I combined both changes into a single one (in the 
next patch).

The changes since v1 are just updated error messages based on the 
comments from https://github.com/checkpoint-restore/criu/pull/843
Andrei Vagin Nov. 18, 2019, 1:04 a.m.
> [PATCHv2 1/4] mount: Add error messages

You need to add space between PATCH and v2, otherwise patchwork thinks
that this is a new series.

https://patchwork.criu.org/series/3860/
https://patchwork.criu.org/series/3875/

And if a series has more than one patch, it is better to sent it with a
cover letter.
Pavel Tikhomirov Nov. 18, 2019, 10:30 a.m.
On 11/17/19 1:29 PM, Radostin Stoyanov wrote:
> On 17/11/2019 09:37, Pavel Tikhomirov wrote:
>> Haven't you lost the "Unable to remount" hunk from v1? And maybe you can
>> write "v2:..." comments so it would be easier to understand what's 
>> changed.
> I'm sorry for the confusion. The same line was also being changed in the 
> following patch, so I combined both changes into a single one (in the 
> next patch).

Looks completely lost in v2:

[snorch@snorch criu]$ git log -p 
criupatchwork/3860~4..criupatchwork/3860 | grep ns_open_mountpoint -A9
@@ -1335,8 +1335,10 @@ int ns_open_mountpoint(void *arg)
  	}

  	/* Remount all mounts as private to disable propagation */
-	if (mount("none", "/", NULL, MS_REC|MS_PRIVATE, NULL))
+	if (mount("none", "/", NULL, MS_REC|MS_PRIVATE, NULL)) {
+		pr_perror("Unable to remount");
  		goto err;
+	}

[snorch@snorch criu]$ git log -p 
criupatchwork/3875~4..criupatchwork/3875 | grep ns_open_mountpoint -A9
[snorch@snorch criu]$

> 
> The changes since v1 are just updated error messages based on the 
> comments from https://github.com/checkpoint-restore/criu/pull/843
Radostin Stoyanov Nov. 18, 2019, 2:02 p.m.
On 18/11/2019 10:30, Pavel Tikhomirov wrote:
> On 11/17/19 1:29 PM, Radostin Stoyanov wrote:
>> On 17/11/2019 09:37, Pavel Tikhomirov wrote:
>>> Haven't you lost the "Unable to remount" hunk from v1? And maybe you can
>>> write "v2:..." comments so it would be easier to understand what's
>>> changed.
>> I'm sorry for the confusion. The same line was also being changed in the
>> following patch, so I combined both changes into a single one (in the
>> next patch).
> Looks completely lost in v2:
>
> [snorch@snorch criu]$ git log -p
> criupatchwork/3860~4..criupatchwork/3860 | grep ns_open_mountpoint -A9
> @@ -1335,8 +1335,10 @@ int ns_open_mountpoint(void *arg)
>    	}
>
>    	/* Remount all mounts as private to disable propagation */
> -	if (mount("none", "/", NULL, MS_REC|MS_PRIVATE, NULL))
> +	if (mount("none", "/", NULL, MS_REC|MS_PRIVATE, NULL)) {
> +		pr_perror("Unable to remount");
>    		goto err;
> +	}
>
> [snorch@snorch criu]$ git log -p
> criupatchwork/3875~4..criupatchwork/3875 | grep ns_open_mountpoint -A9
> [snorch@snorch criu]$
You are right. Thank you for catching that.
I added this change in https://github.com/checkpoint-restore/criu/pull/856

Radostin