[PATCH] nilfs-utils: mkfs.nilfs2 should check presence of NILFS2 volume on device

[PATCH] nilfs-utils: mkfs.nilfs2 should check presence of NILFS2 volume on device

[Vyacheslav Dubeyko]

From: Vyacheslav Dubeyko <slava-yeENwD64cLxBDgjK7y7TUQ@public.gmane.org>
Subject: [PATCH] nilfs-utils: mkfs.nilfs2 should check presence of NILFS2 volume on device

The mkfs.nilfs2 utility should check presence of NILFS2 volume on device and to warn a user about possibility to destroy data by mkfs activity. This patch tries to read and to validate checksums of primary and secondary superblocks on opened device. If this operation ends successfully then mkfs.nilfs2 informs a user about potential danger to destroy existing NILFS2 volume. The execution of mkfs.nilfs2 stops with offering to make decision about continuation or abortion of operation. However, if a user runs mkfs.nilfs2 with "-q" option then checking of NILFS2 volume is skipped.

Reported-by: Hendrik Levsen <hendrik-j5CO6tLloWodnm+yROfE0A@public.gmane.org>
Signed-off-by: Vyacheslav Dubeyko <slava-yeENwD64cLxBDgjK7y7TUQ@public.gmane.org>
Tested-by: Vyacheslav Dubeyko <slava-yeENwD64cLxBDgjK7y7TUQ@public.gmane.org>
---
 sbin/mkfs/Makefile.am |    3 ++-
 sbin/mkfs/mkfs.c      |   34 ++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/sbin/mkfs/Makefile.am b/sbin/mkfs/Makefile.am
index b3eb78a..631b04f 100644
--- a/sbin/mkfs/Makefile.am
+++ b/sbin/mkfs/Makefile.am
@@ -4,7 +4,8 @@ AM_CFLAGS = -Wall
 AM_CPPFLAGS = -I$(top_srcdir)/include
 LDADD = -luuid $(top_builddir)/lib/libnilfsfeature.la \
 	$(top_builddir)/lib/libmountchk.la \
-	$(top_builddir)/lib/libcrc32.la
+	$(top_builddir)/lib/libcrc32.la \
+	$(top_builddir)/lib/libnilfs.la
 
 sbin_PROGRAMS = mkfs.nilfs2
 
diff --git a/sbin/mkfs/mkfs.c b/sbin/mkfs/mkfs.c
index fde1c76..9f18afe 100644
--- a/sbin/mkfs/mkfs.c
+++ b/sbin/mkfs/mkfs.c
@@ -347,6 +347,8 @@ static int nilfs_mkfs_discard_zeroes_data(int fd)
 
 static void disk_scan(const char *device);
 static void check_mount(int fd, const char *device);
+static void check_presence_of_nilfs_volume_on_device(int fd,
+						const char *device);
 
 
 /*
@@ -611,6 +613,7 @@ int main(int argc, char *argv[])
 	if ((fd = open(device, O_RDWR)) < 0)
 		perr("Error: cannot open device: %s", device);
 	check_mount(fd, device);
+	check_presence_of_nilfs_volume_on_device(fd, device);
 
 	init_disk_layout(di, fd, device);
 	si = new_segment(di);
@@ -729,6 +732,37 @@ static void check_mount(int fd, const char *device)
 	fclose(fp);
 }
 
+static void check_presence_of_nilfs_volume_on_device(int fd,
+						const char *device)
+{
+	struct nilfs_super_block *sbp = NULL;
+	int c;
+
+	if (quiet == 0) {
+		sbp = nilfs_sb_read(fd);
+		if (sbp) {
+			free(sbp);
+
+			pinfo("WARNING: Device %s has NILFS2 superblocks.",
+				device);
+			pinfo("WARNING: All data will be lost after format!");
+			pinfo("\nDO YOU REALLY WANT TO FORMAT DEVICE %s?",
+				device);
+
+			do {
+				fprintf(stderr, "\nContinue? [y/N] ");
+				c = getchar();
+
+				if (c == 'n' || c == 'N' || c == EOF) {
+					close(fd);
+					perr("Abort format of device %s",
+						device);
+				}
+			} while (c != 'y' && c != 'Y');
+		}
+	}
+}
+
 static void destroy_disk_buffer(void)
 {
 	if (disk_buffer) {
-- 
1.7.9.5



--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] nilfs-utils: mkfs.nilfs2 should check presence of NILFS2 volume on device

[Ryusuke Konishi]

Hi, Vyacheslav
On Sat, 23 Mar 2013 15:45:22 +0400, Vyacheslav Dubeyko wrote:
> From: Vyacheslav Dubeyko <slava-yeENwD64cLxBDgjK7y7TUQ@public.gmane.org>
> Subject: [PATCH] nilfs-utils: mkfs.nilfs2 should check presence of NILFS2 volume on device
> 
> The mkfs.nilfs2 utility should check presence of NILFS2 volume on device and to warn a user about possibility to destroy data by mkfs activity. This patch tries to read and to validate checksums of primary and secondary superblocks on opened device. If this operation ends successfully then mkfs.nilfs2 informs a user about potential danger to destroy existing NILFS2 volume. The execution of mkfs.nilfs2 stops with offering to make decision about continuation or abortion of operation. However, if a user runs mkfs.nilfs2 with "-q" option then checking of NILFS2 volume is skipped.
> 
> Reported-by: Hendrik Levsen <hendrik-j5CO6tLloWodnm+yROfE0A@public.gmane.org>
> Signed-off-by: Vyacheslav Dubeyko <slava-yeENwD64cLxBDgjK7y7TUQ@public.gmane.org>
> Tested-by: Vyacheslav Dubeyko <slava-yeENwD64cLxBDgjK7y7TUQ@public.gmane.org>

This patch breaks compatibility of mkfs.nilfs2 in the case when users
or tools are expecting overwrite an exisiting partition.  However, I
am inclined to accept this kind of protection feature.

One thing disturbing is that "quiet option" is used to turn off the
protection; "quiet option" is just an option to suppress messages.

Please consider adding "force overwrite (-f)" option like
mkfs.xfs.

Thanks,
Ryusuke Konishi

> ---
>  sbin/mkfs/Makefile.am |    3 ++-
>  sbin/mkfs/mkfs.c      |   34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/sbin/mkfs/Makefile.am b/sbin/mkfs/Makefile.am
> index b3eb78a..631b04f 100644
> --- a/sbin/mkfs/Makefile.am
> +++ b/sbin/mkfs/Makefile.am
> @@ -4,7 +4,8 @@ AM_CFLAGS = -Wall
>  AM_CPPFLAGS = -I$(top_srcdir)/include
>  LDADD = -luuid $(top_builddir)/lib/libnilfsfeature.la \
>  	$(top_builddir)/lib/libmountchk.la \
> -	$(top_builddir)/lib/libcrc32.la
> +	$(top_builddir)/lib/libcrc32.la \
> +	$(top_builddir)/lib/libnilfs.la
>  
>  sbin_PROGRAMS = mkfs.nilfs2
>  
> diff --git a/sbin/mkfs/mkfs.c b/sbin/mkfs/mkfs.c
> index fde1c76..9f18afe 100644
> --- a/sbin/mkfs/mkfs.c
> +++ b/sbin/mkfs/mkfs.c
> @@ -347,6 +347,8 @@ static int nilfs_mkfs_discard_zeroes_data(int fd)
>  
>  static void disk_scan(const char *device);
>  static void check_mount(int fd, const char *device);
> +static void check_presence_of_nilfs_volume_on_device(int fd,
> +						const char *device);
>  
>  
>  /*
> @@ -611,6 +613,7 @@ int main(int argc, char *argv[])
>  	if ((fd = open(device, O_RDWR)) < 0)
>  		perr("Error: cannot open device: %s", device);
>  	check_mount(fd, device);
> +	check_presence_of_nilfs_volume_on_device(fd, device);
>  
>  	init_disk_layout(di, fd, device);
>  	si = new_segment(di);
> @@ -729,6 +732,37 @@ static void check_mount(int fd, const char *device)
>  	fclose(fp);
>  }
>  
> +static void check_presence_of_nilfs_volume_on_device(int fd,
> +						const char *device)
> +{
> +	struct nilfs_super_block *sbp = NULL;
> +	int c;
> +
> +	if (quiet == 0) {
> +		sbp = nilfs_sb_read(fd);
> +		if (sbp) {
> +			free(sbp);
> +
> +			pinfo("WARNING: Device %s has NILFS2 superblocks.",
> +				device);
> +			pinfo("WARNING: All data will be lost after format!");
> +			pinfo("\nDO YOU REALLY WANT TO FORMAT DEVICE %s?",
> +				device);
> +
> +			do {
> +				fprintf(stderr, "\nContinue? [y/N] ");
> +				c = getchar();
> +
> +				if (c == 'n' || c == 'N' || c == EOF) {
> +					close(fd);
> +					perr("Abort format of device %s",
> +						device);
> +				}
> +			} while (c != 'y' && c != 'Y');
> +		}
> +	}
> +}
> +
>  static void destroy_disk_buffer(void)
>  {
>  	if (disk_buffer) {
> -- 
> 1.7.9.5
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] nilfs-utils: mkfs.nilfs2 should check presence of NILFS2 volume on device

[Martin Steigerwald]

Hi!

Am Samstag, 23. März 2013 schrieb Ryusuke Konishi:
> On Sat, 23 Mar 2013 15:45:22 +0400, Vyacheslav Dubeyko wrote:
> > From: Vyacheslav Dubeyko <slava-yeENwD64cLxBDgjK7y7TUQ@public.gmane.org>
> > Subject: [PATCH] nilfs-utils: mkfs.nilfs2 should check presence of
> > NILFS2 volume on device
> > 
> > The mkfs.nilfs2 utility should check presence of NILFS2 volume on
> > device and to warn a user about possibility to destroy data by mkfs
> > activity. This patch tries to read and to validate checksums of
> > primary and secondary superblocks on opened device. If this operation
> > ends successfully then mkfs.nilfs2 informs a user about potential
> > danger to destroy existing NILFS2 volume. The execution of mkfs.nilfs2
> > stops with offering to make decision about continuation or abortion of
> > operation. However, if a user runs mkfs.nilfs2 with "-q" option then
> > checking of NILFS2 volume is skipped.
> > 
> > Reported-by: Hendrik Levsen <hendrik-j5CO6tLloWodnm+yROfE0A@public.gmane.org>
> > Signed-off-by: Vyacheslav Dubeyko <slava-yeENwD64cLxBDgjK7y7TUQ@public.gmane.org>
> > Tested-by: Vyacheslav Dubeyko <slava-yeENwD64cLxBDgjK7y7TUQ@public.gmane.org>
> 
> This patch breaks compatibility of mkfs.nilfs2 in the case when users
> or tools are expecting overwrite an exisiting partition.  However, I
> am inclined to accept this kind of protection feature.
> 
> One thing disturbing is that "quiet option" is used to turn off the
> protection; "quiet option" is just an option to suppress messages.
> 
> Please consider adding "force overwrite (-f)" option like
> mkfs.xfs.

Also please consider adding of other filesystems via blkid just as mkfs.xfs 
and since a short time also mkfs.btrfs does.

I agree on "-f" being a suitable option for force formatting the volume.

Thanks,
-- 
Martin 'Helios' Steigerwald - http://www.Lichtvoll.de
GPG: 03B0 0D6C 0040 0710 4AFA  B82F 991B EAAC A599 84C7
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] nilfs-utils: mkfs.nilfs2 should check presence of NILFS2 volume on device

[Vyacheslav Dubeyko]

Hi Martin,

On Mar 24, 2013, at 3:09 PM, Martin Steigerwald wrote:

> Hi!
> 
> Am Samstag, 23. März 2013 schrieb Ryusuke Konishi:
>> On Sat, 23 Mar 2013 15:45:22 +0400, Vyacheslav Dubeyko wrote:
>>> From: Vyacheslav Dubeyko <slava-yeENwD64cLxBDgjK7y7TUQ@public.gmane.org>
>>> Subject: [PATCH] nilfs-utils: mkfs.nilfs2 should check presence of
>>> NILFS2 volume on device
>>> 
>>> The mkfs.nilfs2 utility should check presence of NILFS2 volume on
>>> device and to warn a user about possibility to destroy data by mkfs
>>> activity. This patch tries to read and to validate checksums of
>>> primary and secondary superblocks on opened device. If this operation
>>> ends successfully then mkfs.nilfs2 informs a user about potential
>>> danger to destroy existing NILFS2 volume. The execution of mkfs.nilfs2
>>> stops with offering to make decision about continuation or abortion of
>>> operation. However, if a user runs mkfs.nilfs2 with "-q" option then
>>> checking of NILFS2 volume is skipped.
>>> 
>>> Reported-by: Hendrik Levsen <hendrik-j5CO6tLloWodnm+yROfE0A@public.gmane.org>
>>> Signed-off-by: Vyacheslav Dubeyko <slava-yeENwD64cLxBDgjK7y7TUQ@public.gmane.org>
>>> Tested-by: Vyacheslav Dubeyko <slava-yeENwD64cLxBDgjK7y7TUQ@public.gmane.org>
>> 
>> This patch breaks compatibility of mkfs.nilfs2 in the case when users
>> or tools are expecting overwrite an exisiting partition.  However, I
>> am inclined to accept this kind of protection feature.
>> 
>> One thing disturbing is that "quiet option" is used to turn off the
>> protection; "quiet option" is just an option to suppress messages.
>> 
>> Please consider adding "force overwrite (-f)" option like
>> mkfs.xfs.
> 
> Also please consider adding of other filesystems via blkid just as mkfs.xfs 
> and since a short time also mkfs.btrfs does.
> 

Sorry, I don't fully understand about mkfs.btrfs. Could you describe in more details what you mean?

Thanks,
Vyacheslav Dubeyko.

> I agree on "-f" being a suitable option for force formatting the volume.
> 
> Thanks,
> -- 
> Martin 'Helios' Steigerwald - http://www.Lichtvoll.de
> GPG: 03B0 0D6C 0040 0710 4AFA  B82F 991B EAAC A599 84C7

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] nilfs-utils: mkfs.nilfs2 should check presence of NILFS2 volume on device

[Martin Steigerwald]

Am Sonntag, 24. März 2013 schrieb Vyacheslav Dubeyko:
> Hi Martin,

Hi Vyacheslav, Eric and everyone else,

Eric, put you on Cc, cause you wrote mkfs.btrfs -f patch...

> On Mar 24, 2013, at 3:09 PM, Martin Steigerwald wrote:
> > Hi!
> > 
> > Am Samstag, 23. März 2013 schrieb Ryusuke Konishi:
> >> On Sat, 23 Mar 2013 15:45:22 +0400, Vyacheslav Dubeyko wrote:
> >>> From: Vyacheslav Dubeyko <slava-yeENwD64cLxBDgjK7y7TUQ@public.gmane.org>
> >>> Subject: [PATCH] nilfs-utils: mkfs.nilfs2 should check presence of
> >>> NILFS2 volume on device
> >>> 
> >>> The mkfs.nilfs2 utility should check presence of NILFS2 volume on
> >>> device and to warn a user about possibility to destroy data by mkfs
> >>> activity. This patch tries to read and to validate checksums of
> >>> primary and secondary superblocks on opened device. If this operation
> >>> ends successfully then mkfs.nilfs2 informs a user about potential
> >>> danger to destroy existing NILFS2 volume. The execution of
> >>> mkfs.nilfs2 stops with offering to make decision about continuation
> >>> or abortion of operation. However, if a user runs mkfs.nilfs2 with
> >>> "-q" option then checking of NILFS2 volume is skipped.
> >>> 
> >>> Reported-by: Hendrik Levsen <hendrik-j5CO6tLloWodnm+yROfE0A@public.gmane.org>
> >>> Signed-off-by: Vyacheslav Dubeyko <slava-yeENwD64cLxBDgjK7y7TUQ@public.gmane.org>
> >>> Tested-by: Vyacheslav Dubeyko <slava-yeENwD64cLxBDgjK7y7TUQ@public.gmane.org>
> >> 
> >> This patch breaks compatibility of mkfs.nilfs2 in the case when users
> >> or tools are expecting overwrite an exisiting partition.  However, I
> >> am inclined to accept this kind of protection feature.
> >> 
> >> One thing disturbing is that "quiet option" is used to turn off the
> >> protection; "quiet option" is just an option to suppress messages.
> >> 
> >> Please consider adding "force overwrite (-f)" option like
> >> mkfs.xfs.
> > 
> > Also please consider adding of other filesystems via blkid just as
> > mkfs.xfs and since a short time also mkfs.btrfs does.
> 
> Sorry, I don't fully understand about mkfs.btrfs. Could you describe in
> more details what you mean?

mkfs.xfs was the first and to my knowledge long time the only mkfs which 
checked for existing filesystems. And it does so via blkid (at least 
meanwhile, I think it used different means initially). This way it detects 
all current Linux filesystems without duplicating the detection code. blkid 
also detects nilfs:

merkaba:~> mkfs.nilfs2 -L test /dev/merkaba/zeit
mkfs.nilfs2 (nilfs-utils 2.1.4)
Start writing file system initial data to the device
       Blocksize:4096  Device:/dev/merkaba/zeit  Device Size:21474836480
File system initialization succeeded !! 
merkaba:~> blkid /dev/merkaba/zeit
/dev/merkaba/zeit: LABEL="test" UUID="3a105634-638e-4288-8f39-0e5324361431" 
TYPE="nilfs2" 

On some occasion the thought came up that it would be good to have a similar 
behavior in mkfs.btrfs. And mkfs.btrfs basically got a similar 
implementation provided by Eric Sandeen. I digged it out, neither 
ixquick.com nor Google found it out of the box and gmane search also didn´t 
yield a result. Thus manually browsed:

http://www.mail-archive.com/linux-btrfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg22216.html

Ciao
-- 
Martin 'Helios' Steigerwald - http://www.Lichtvoll.de
GPG: 03B0 0D6C 0040 0710 4AFA  B82F 991B EAAC A599 84C7
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] nilfs-utils: mkfs.nilfs2 should check presence of NILFS2 volume on device

[Vyacheslav Dubeyko]

Hi Martin,

On Mar 24, 2013, at 5:43 PM, Martin Steigerwald wrote:
[snip]

>> 
> 
> mkfs.xfs was the first and to my knowledge long time the only mkfs which 
> checked for existing filesystems. And it does so via blkid (at least 
> meanwhile, I think it used different means initially). This way it detects 
> all current Linux filesystems without duplicating the detection code. blkid 
> also detects nilfs:
> 
> merkaba:~> mkfs.nilfs2 -L test /dev/merkaba/zeit
> mkfs.nilfs2 (nilfs-utils 2.1.4)
> Start writing file system initial data to the device
>       Blocksize:4096  Device:/dev/merkaba/zeit  Device Size:21474836480
> File system initialization succeeded !! 
> merkaba:~> blkid /dev/merkaba/zeit
> /dev/merkaba/zeit: LABEL="test" UUID="3a105634-638e-4288-8f39-0e5324361431" 
> TYPE="nilfs2" 
> 
> On some occasion the thought came up that it would be good to have a similar 
> behavior in mkfs.btrfs. And mkfs.btrfs basically got a similar 
> implementation provided by Eric Sandeen. I digged it out, neither 
> ixquick.com nor Google found it out of the box and gmane search also didn´t 
> yield a result. Thus manually browsed:
> 
> http://www.mail-archive.com/linux-btrfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg22216.html
> 

Thank you for detailed explanation and for the link.

Yes, using of libblkid is an excellent suggestion. As I see, it exists an example for mkfs utilities in libblkid library. So, I'll do it in the way that you suggest.

Thanks,
Vyacheslav Dubeyko.

> Ciao
> -- 
> Martin 'Helios' Steigerwald - http://www.Lichtvoll.de
> GPG: 03B0 0D6C 0040 0710 4AFA  B82F 991B EAAC A599 84C7

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] nilfs-utils: mkfs.nilfs2 should check presence of NILFS2 volume on device

[Eric Sandeen]

On 3/24/13 11:08 AM, Vyacheslav Dubeyko wrote:
> Yes, using of libblkid is an excellent suggestion. As I see, it
> exists an example for mkfs utilities in libblkid library. So, I'll do
> it in the way that you suggest.

Yes, if you want to do this, it's pretty easy.  Here is the btrfs
commit:

http://git.kernel.org/cgit/linux/kernel/git/mason/btrfs-progs.git/commit/?id=2a2d8e1962e8b6cda7b0a7584f6d2fb95d442cb6

some people liked it, some did not like it at all, but in the end the option stayed in.

-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html