mkfs.gfs2: Follow symlinks before checking device contents

----- Original Message -----
| + absname = canonicalize_file_name(sdp->device_name);

Hi Andy,

Thanks for the patch. I just wanted to point out that in the past we've used realpath rather than canonicalize_file_name. For example, see this patch we did a long time ago to gfs2_tool:

http://git.fedorahosted.org/git/?p=cluster.git;a=commitdiff;h=e70898cfa09939a7100a057433fff3a4ad666bdd

It would be nice if our use was consistent. I'm not sure if there's an advantage of one over the other. If canonicalize_file_name is now preferred upstream over realpath, we should probably replace all occurrences of that.

On the other hand, if realpath is now preferred upstream, we should adjust this patch to use it instead. AFAIK, they are the same, and I don't have a personal preference; whatever is most favoured by the upstream community. :)

Otherwise, the patch looks good.

----- Original Message -----
| + absname = canonicalize_file_name(sdp->device_name);

Hi Andy,

Thanks for the patch. I just wanted to point out that in the past we've used realpath rather than canonicalize_file_name. For example, see this patch we did a long time ago to gfs2_tool:

http://git.fedorahosted.org/git/?p=cluster.git;a=commitdiff;h=e70898cfa09939a7100a057433fff3a4ad666bdd

It would be nice if our use was consistent. I'm not sure if there's an advantage of one over the other. If canonicalize_file_name is now preferred upstream over realpath, we should probably replace all occurrences of that.

On the other hand, if realpath is now preferred upstream, we should adjust this patch to use it instead. AFAIK, they are the same, and I don't have a personal preference; whatever is most favoured by the upstream community. :)

Otherwise, the patch looks good.

When using symlinks with mkfs.gfs2 it reports what the symlink points to instead of the contents of the device, like so:

 # ./mkfs.gfs2 -p lock_nolock /dev/vg/test
 This will destroy any data on /dev/vg/test.
 It appears to contain: symbolic link to `../dm-3'

This patch resolves symlinks before checking the device contents to make the output more informative:

 # ./mkfs.gfs2 -p lock_nolock /dev/vg/test
 This will destroy any data on /dev/vg/test.
 It appears to contain: GFS2 Filesystem (blocksize 4096, lockproto lock_nolock)

Signed-off-by Andrew Price <[email protected]>

---
 gfs2/mkfs/main_mkfs.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

gfs2/mkfs/main_mkfs.c +8 -1

index e6b00a0..f8e4741 100644
@@ -531,6 +531,7 @@ void main_mkfs(int argc, char *argv[])
 	int error;
 	int rgsize_specified = 0;
 	unsigned char uuid[16];
+	char *absname;
 
 	memset(sdp, 0, sizeof(struct gfs2_sbd));
 	sdp->bsize = -1;

@@ -560,11 +561,17 @@ void main_mkfs(int argc, char *argv[])
 		exit(EXIT_FAILURE);
 	}
 
+	absname = canonicalize_file_name(sdp->device_name);
+	if (absname == NULL) {
+		perror(_("Could not find the absolute path of the device"));
+		exit(EXIT_FAILURE);
+	}
 	if (!sdp->override) {
 		printf( _("This will destroy any data on %s.\n"), sdp->device_name);
-		check_dev_content(sdp->device_name);
+		check_dev_content(absname);
 		are_you_sure();
 	}
+	free(absname);
 
 	if (sdp->bsize == -1) {
 		if (S_ISREG(sdp->dinfo.stat.st_mode))

Hi,

Looks good. ACK,

Steve.

Quoted 50 lines · Andrew Price

When using symlinks with mkfs.gfs2 it reports what the symlink points to instead of the contents of the device, like so:

  # ./mkfs.gfs2 -p lock_nolock /dev/vg/test
  This will destroy any data on /dev/vg/test.
  It appears to contain: symbolic link to `../dm-3'

This patch resolves symlinks before checking the device contents to make the output more informative:

  # ./mkfs.gfs2 -p lock_nolock /dev/vg/test
  This will destroy any data on /dev/vg/test.
  It appears to contain: GFS2 Filesystem (blocksize 4096, lockproto lock_nolock)

Signed-off-by Andrew Price <[email protected]>

---
  gfs2/mkfs/main_mkfs.c |    9 ++++++++-
  1 files changed, 8 insertions(+), 1 deletions(-)

gfs2/mkfs/main_mkfs.c +8 -1

index e6b00a0..f8e4741 100644
@@ -531,6 +531,7 @@ void main_mkfs(int argc, char *argv[])
  	int error;
  	int rgsize_specified = 0;
  	unsigned char uuid[16];
+	char *absname;
 
  	memset(sdp, 0, sizeof(struct gfs2_sbd));
  	sdp->bsize = -1;

@@ -560,11 +561,17 @@ void main_mkfs(int argc, char *argv[])
  		exit(EXIT_FAILURE);
  	}
 
+	absname = canonicalize_file_name(sdp->device_name);
+	if (absname == NULL) {
+		perror(_("Could not find the absolute path of the device"));
+		exit(EXIT_FAILURE);
+	}
  	if (!sdp->override) {
  		printf( _("This will destroy any data on %s.\n"), sdp->device_name);
-		check_dev_content(sdp->device_name);
+		check_dev_content(absname);
  		are_you_sure();
  	}
+	free(absname);
 
  	if (sdp->bsize == -1) {
  		if (S_ISREG(sdp->dinfo.stat.st_mode))

Hi Steve,

Quoted 3 lines · Steven Whitehouse

Hi,

Looks good. ACK,

This one was obsoleted by the v2 patch I posted on the 21st. If you prefer this one though I can push it instead.

Andy

Quoted 56 lines

Steve.

Quoted 50 lines · Andrew Price

When using symlinks with mkfs.gfs2 it reports what the symlink points to instead of the contents of the device, like so:

  # ./mkfs.gfs2 -p lock_nolock /dev/vg/test
  This will destroy any data on /dev/vg/test.
  It appears to contain: symbolic link to `../dm-3'

This patch resolves symlinks before checking the device contents to make the output more informative:

  # ./mkfs.gfs2 -p lock_nolock /dev/vg/test
  This will destroy any data on /dev/vg/test.
  It appears to contain: GFS2 Filesystem (blocksize 4096, lockproto lock_nolock)

Signed-off-by Andrew Price <[email protected]>

---
  gfs2/mkfs/main_mkfs.c |    9 ++++++++-
  1 files changed, 8 insertions(+), 1 deletions(-)

gfs2/mkfs/main_mkfs.c +8 -1

index e6b00a0..f8e4741 100644
@@ -531,6 +531,7 @@ void main_mkfs(int argc, char *argv[])
  	int error;
  	int rgsize_specified = 0;
  	unsigned char uuid[16];
+	char *absname;
 
  	memset(sdp, 0, sizeof(struct gfs2_sbd));
  	sdp->bsize = -1;

@@ -560,11 +561,17 @@ void main_mkfs(int argc, char *argv[])
  		exit(EXIT_FAILURE);
  	}
 
+	absname = canonicalize_file_name(sdp->device_name);
+	if (absname == NULL) {
+		perror(_("Could not find the absolute path of the device"));
+		exit(EXIT_FAILURE);
+	}
  	if (!sdp->override) {
  		printf( _("This will destroy any data on %s.\n"), sdp->device_name);
-		check_dev_content(sdp->device_name);
+		check_dev_content(absname);
  		are_you_sure();
  	}
+	free(absname);
 
  	if (sdp->bsize == -1) {
  		if (S_ISREG(sdp->dinfo.stat.st_mode))

Hi,

Yes, v2 is ok too. I think I missed that in my first pass over my email today. To do this properly, we should be opening the path that we are passed, and then doing the remaining operations with just the fd. We could do that with "file" if we used the -f - option and passed the fd as standard input to the process.

That way we also remove any possible race conditions too, but this is good enough for now,

Steve.

Quoted 70 lines · Andrew Price

Hi Steve,

Quoted 3 lines · Steven Whitehouse

Hi,

Looks good. ACK,

This one was obsoleted by the v2 patch I posted on the 21st. If you prefer this one though I can push it instead.

Andy

Quoted 56 lines

Steve.

Quoted 50 lines · Andrew Price

When using symlinks with mkfs.gfs2 it reports what the symlink points to instead of the contents of the device, like so:

   # ./mkfs.gfs2 -p lock_nolock /dev/vg/test
   This will destroy any data on /dev/vg/test.
   It appears to contain: symbolic link to `../dm-3'

This patch resolves symlinks before checking the device contents to make the output more informative:

   # ./mkfs.gfs2 -p lock_nolock /dev/vg/test
   This will destroy any data on /dev/vg/test.
   It appears to contain: GFS2 Filesystem (blocksize 4096, lockproto lock_nolock)

Signed-off-by Andrew Price <[email protected]>

---
   gfs2/mkfs/main_mkfs.c |    9 ++++++++-
   1 files changed, 8 insertions(+), 1 deletions(-)

gfs2/mkfs/main_mkfs.c +8 -1

index e6b00a0..f8e4741 100644
@@ -531,6 +531,7 @@ void main_mkfs(int argc, char *argv[])
   	int error;
   	int rgsize_specified = 0;
   	unsigned char uuid[16];
+	char *absname;
 
   	memset(sdp, 0, sizeof(struct gfs2_sbd));
   	sdp->bsize = -1;

@@ -560,11 +561,17 @@ void main_mkfs(int argc, char *argv[])
   		exit(EXIT_FAILURE);
   	}
 
+	absname = canonicalize_file_name(sdp->device_name);
+	if (absname == NULL) {
+		perror(_("Could not find the absolute path of the device"));
+		exit(EXIT_FAILURE);
+	}
   	if (!sdp->override) {
   		printf( _("This will destroy any data on %s.\n"), sdp->device_name);
-		check_dev_content(sdp->device_name);
+		check_dev_content(absname);
   		are_you_sure();
   	}
+	free(absname);
 
   	if (sdp->bsize == -1) {
   		if (S_ISREG(sdp->dinfo.stat.st_mode))

Quoted 7 lines · Steven Whitehouse

Hi,

Yes, v2 is ok too. I think I missed that in my first pass over my email today. To do this properly, we should be opening the path that we are passed, and then doing the remaining operations with just the fd. We could do that with "file" if we used the -f - option and passed the fd as standard input to the process.

The only way I can think of passing the fd to file -f - would be the C equivalent of:

   printf "/proc/$pid/fd/$fd" | file -f -

Which would mean adding another pipe() and fork(). So we might as well just shorten it to:

   file "/proc/$pid/fd/$fd"

Would this solve the race condition problem? Or am I misunderstanding how you intended it to be done?

Quoted 82 lines

That way we also remove any possible race conditions too, but this is good enough for now,

Steve.

Quoted 73 lines · Andrew Price

Hi Steve,

Quoted 3 lines · Steven Whitehouse

Hi,

Looks good. ACK,

This one was obsoleted by the v2 patch I posted on the 21st. If you prefer this one though I can push it instead.

Andy

Quoted 59 lines

Steve.

Quoted 53 lines · Andrew Price

When using symlinks with mkfs.gfs2 it reports what the symlink points to instead of the contents of the device, like so:

   # ./mkfs.gfs2 -p lock_nolock /dev/vg/test
   This will destroy any data on /dev/vg/test.
   It appears to contain: symbolic link to `../dm-3'

This patch resolves symlinks before checking the device contents to make
the output more informative:

   # ./mkfs.gfs2 -p lock_nolock /dev/vg/test
   This will destroy any data on /dev/vg/test.
   It appears to contain: GFS2 Filesystem (blocksize 4096,
lockproto lock_nolock)

Signed-off-by Andrew Price <[email protected]>

---
   gfs2/mkfs/main_mkfs.c |    9 ++++++++-
   1 files changed, 8 insertions(+), 1 deletions(-)

gfs2/mkfs/main_mkfs.c +8 -1

index e6b00a0..f8e4741 100644
@@ -531,6 +531,7 @@ void main_mkfs(int argc, char *argv[])
       int error;
       int rgsize_specified = 0;
       unsigned char uuid[16];
+    char *absname;
 
       memset(sdp, 0, sizeof(struct gfs2_sbd));
       sdp->bsize = -1;

@@ -560,11 +561,17 @@ void main_mkfs(int argc, char *argv[])
           exit(EXIT_FAILURE);
       }
 
+    absname = canonicalize_file_name(sdp->device_name);
+    if (absname == NULL) {
+        perror(_("Could not find the absolute path of the device"));
+        exit(EXIT_FAILURE);
+    }
       if (!sdp->override) {
           printf( _("This will destroy any data on %s.\n"),
 sdp->device_name);
-        check_dev_content(sdp->device_name);
+        check_dev_content(absname);
           are_you_sure();
       }
+    free(absname);
 
       if (sdp->bsize == -1) {

           if (S_ISREG(sdp->dinfo.stat.st_mode))

Quoted 19 lines · Andrew Price

Quoted 7 lines · Steven Whitehouse

Hi,

Yes, v2 is ok too. I think I missed that in my first pass over my email today. To do this properly, we should be opening the path that we are passed, and then doing the remaining operations with just the fd. We could do that with "file" if we used the -f - option and passed the fd as standard input to the process.

The only way I can think of passing the fd to file -f - would be the C equivalent of:

   printf "/proc/$pid/fd/$fd" | file -f -

Which would mean adding another pipe() and fork(). So we might as well just shorten it to:

   file "/proc/$pid/fd/$fd"

Quoted 2 lines

Would this solve the race condition problem? Or am I misunderstanding how you intended it to be done?

I didn't get a reply to this. Does the above method seem sensible or would we need to read the start of the file/device ourselves and pipe it into 'file -' to avoid opening the file twice?

Andy

Quoted 84 lines

Quoted 82 lines

That way we also remove any possible race conditions too, but this is good enough for now,

Steve.

Quoted 73 lines · Andrew Price

Hi Steve,

Quoted 3 lines · Steven Whitehouse

Hi,

Looks good. ACK,

This one was obsoleted by the v2 patch I posted on the 21st. If you prefer this one though I can push it instead.

Andy

Quoted 59 lines

Steve.

Quoted 53 lines · Andrew Price

When using symlinks with mkfs.gfs2 it reports what the symlink points to instead of the contents of the device, like so:

   # ./mkfs.gfs2 -p lock_nolock /dev/vg/test
   This will destroy any data on /dev/vg/test.
   It appears to contain: symbolic link to `../dm-3'

This patch resolves symlinks before checking the device contents to make
the output more informative:

   # ./mkfs.gfs2 -p lock_nolock /dev/vg/test
   This will destroy any data on /dev/vg/test.
   It appears to contain: GFS2 Filesystem (blocksize 4096,
lockproto lock_nolock)

Signed-off-by Andrew Price <[email protected]>

---
   gfs2/mkfs/main_mkfs.c |    9 ++++++++-
   1 files changed, 8 insertions(+), 1 deletions(-)

gfs2/mkfs/main_mkfs.c +8 -1

index e6b00a0..f8e4741 100644
@@ -531,6 +531,7 @@ void main_mkfs(int argc, char *argv[])
       int error;
       int rgsize_specified = 0;
       unsigned char uuid[16];
+    char *absname;
 
       memset(sdp, 0, sizeof(struct gfs2_sbd));
       sdp->bsize = -1;

@@ -560,11 +561,17 @@ void main_mkfs(int argc, char *argv[])
           exit(EXIT_FAILURE);
       }
 
+    absname = canonicalize_file_name(sdp->device_name);
+    if (absname == NULL) {
+        perror(_("Could not find the absolute path of the device"));
+        exit(EXIT_FAILURE);
+    }
       if (!sdp->override) {
           printf( _("This will destroy any data on %s.\n"),
 sdp->device_name);
-        check_dev_content(sdp->device_name);
+        check_dev_content(absname);
           are_you_sure();
       }
+    free(absname);
 
       if (sdp->bsize == -1) {

           if (S_ISREG(sdp->dinfo.stat.st_mode))