Util-Linux Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] blkdiscard: Refuse to proceed if signatures are found
@ 2020-06-18  9:29 Lukas Czerner
  2020-06-18 10:06 ` Karel Zak
  0 siblings, 1 reply; 3+ messages in thread
From: Lukas Czerner @ 2020-06-18  9:29 UTC (permalink / raw)
  To: util-linux; +Cc: kzak

Currently the blkdiscard has the ability to wipe out entere device in a
matter of seconds. This is fine as long as it's intentional, it is
potentially catastrophic if it's not.

With this commit blkdiscard will check for existing signatures on the
device and refuse to continue if any are found unless the operation is
forced with the -f option.

In an attempt to avoid breaking existing automation scripts the force is
only required when stdin refers to a terminal.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 sys-utils/Makemodule.am |  2 +-
 sys-utils/blkdiscard.c  | 54 +++++++++++++++++++++++++++++++++++++++--
 2 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/sys-utils/Makemodule.am b/sys-utils/Makemodule.am
index 5855e1cc1..b5f6c1b1b 100644
--- a/sys-utils/Makemodule.am
+++ b/sys-utils/Makemodule.am
@@ -165,7 +165,7 @@ if BUILD_BLKDISCARD
 sbin_PROGRAMS += blkdiscard
 dist_man_MANS += sys-utils/blkdiscard.8
 blkdiscard_SOURCES = sys-utils/blkdiscard.c lib/monotonic.c
-blkdiscard_LDADD = $(LDADD) libcommon.la $(REALTIME_LIBS)
+blkdiscard_LDADD = $(LDADD) libblkid.la libcommon.la $(REALTIME_LIBS)
 endif
 
 if BUILD_BLKZONE
diff --git a/sys-utils/blkdiscard.c b/sys-utils/blkdiscard.c
index e83f69b11..f9eb6b7b0 100644
--- a/sys-utils/blkdiscard.c
+++ b/sys-utils/blkdiscard.c
@@ -37,6 +37,7 @@
 #include <sys/stat.h>
 #include <sys/time.h>
 #include <linux/fs.h>
+#include <blkid/blkid.h>
 
 #include "nls.h"
 #include "strutils.h"
@@ -106,11 +107,45 @@ static void __attribute__((__noreturn__)) usage(void)
 	exit(EXIT_SUCCESS);
 }
 
+/*
+ * Check existing signature on the open fd
+ * Returns	0  if no signature was found
+ * 		1  if a signature was found
+ * 		<0 on error
+ */
+static int device_empty(int fd, char *path)
+{
+	const char *type;
+	blkid_probe pr = NULL;
+	int ret = -1;
+
+	pr = blkid_new_probe();
+	if (!pr || blkid_probe_set_device(pr, fd, 0, 0))
+		return ret;
+
+	blkid_probe_enable_superblocks(pr, TRUE);
+	blkid_probe_enable_partitions(pr, TRUE);
+
+	ret = blkid_do_fullprobe(pr);
+	if (ret)
+		return ret;
+
+	if (!blkid_probe_lookup_value(pr, "TYPE", &type, NULL)) {
+		warnx("%s contains existing file system (%s).",path ,type);
+	} else if (!blkid_probe_lookup_value(pr, "PTTYPE", &type, NULL)) {
+		warnx("%s contains existing partition (%s).",path ,type);
+	} else {
+		warnx("%s contains existing signature.", path);
+	}
+
+	blkid_free_probe(pr);
+	return ret;
+}
 
 int main(int argc, char **argv)
 {
 	char *path;
-	int c, fd, verbose = 0, secsize, force = 0;
+	int c, fd, ret, verbose = 0, secsize, force = 0;
 	uint64_t end, blksize, step, range[2], stats[2];
 	struct stat sb;
 	struct timeval now, last;
@@ -184,7 +219,7 @@ int main(int argc, char **argv)
 		errtryhelp(EXIT_FAILURE);
 	}
 
-	fd = open(path, O_WRONLY | (force ? 0 : O_EXCL));
+	fd = open(path, O_RDWR | (force ? 0 : O_EXCL));
 	if (fd < 0)
 		err(EXIT_FAILURE, _("cannot open %s"), path);
 
@@ -217,6 +252,21 @@ int main(int argc, char **argv)
 		errx(EXIT_FAILURE, _("%s: length %" PRIu64 " is not aligned "
 			 "to sector size %i"), path, range[1], secsize);
 
+	 /* Check for existing signatures on the device */
+	if ((ret = device_empty(fd, path)) == 0) {
+		/*
+		 * Only require force in interactive mode to avoid
+		 * breaking existing scripts
+		 */
+		if (!force && isatty(STDIN_FILENO)) {
+			errx(EXIT_FAILURE,
+			     _("This is destructive operation, data will " \
+			       "be lost! Use the -f option to override."));
+		}
+		warnx(_("Operation forced, data will be lost!"));
+	} else if (ret < 0)
+		err(EXIT_FAILURE, _("failed to probe the device"));
+
 	stats[0] = range[0], stats[1] = 0;
 	gettime_monotonic(&last);
 
-- 
2.21.3


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] blkdiscard: Refuse to proceed if signatures are found
  2020-06-18  9:29 [PATCH] blkdiscard: Refuse to proceed if signatures are found Lukas Czerner
@ 2020-06-18 10:06 ` Karel Zak
  2020-06-18 10:33   ` Lukas Czerner
  0 siblings, 1 reply; 3+ messages in thread
From: Karel Zak @ 2020-06-18 10:06 UTC (permalink / raw)
  To: Lukas Czerner; +Cc: util-linux

On Thu, Jun 18, 2020 at 11:29:16AM +0200, Lukas Czerner wrote:
> With this commit blkdiscard will check for existing signatures on the
> device and refuse to continue if any are found unless the operation is
> forced with the -f option.

Good idea.

> +/*
> + * Check existing signature on the open fd
> + * Returns	0  if no signature was found
> + * 		1  if a signature was found

this is not true, 0 means detected, 1 not found

> + * 		<0 on error
> + */
> +static int device_empty(int fd, char *path)

This is difficult for to read, at first glance it seems according
to function name that 1 means "yes, it's empty".
      
Maybe rename it to probe_device().

> +{
> +	const char *type;
> +	blkid_probe pr = NULL;
> +	int ret = -1;
> +
> +	pr = blkid_new_probe();
> +	if (!pr || blkid_probe_set_device(pr, fd, 0, 0))
> +		return ret;
> +
> +	blkid_probe_enable_superblocks(pr, TRUE);
> +	blkid_probe_enable_partitions(pr, TRUE);
> +
> +	ret = blkid_do_fullprobe(pr);
> +	if (ret)
> +		return ret;

yes, blkid_do_fullprobe() returns: 0 on success, 1 if nothing is detected or -1 on case of error.

> +
> +	if (!blkid_probe_lookup_value(pr, "TYPE", &type, NULL)) {
> +		warnx("%s contains existing file system (%s).",path ,type);
> +	} else if (!blkid_probe_lookup_value(pr, "PTTYPE", &type, NULL)) {
> +		warnx("%s contains existing partition (%s).",path ,type);
> +	} else {
> +		warnx("%s contains existing signature.", path);
> +	}
> +
> +	blkid_free_probe(pr);
> +	return ret;

This is always 0.

> +}
>  
>  int main(int argc, char **argv)
>  {
>  	char *path;
> -	int c, fd, verbose = 0, secsize, force = 0;
> +	int c, fd, ret, verbose = 0, secsize, force = 0;
>  	uint64_t end, blksize, step, range[2], stats[2];
>  	struct stat sb;
>  	struct timeval now, last;
> @@ -184,7 +219,7 @@ int main(int argc, char **argv)
>  		errtryhelp(EXIT_FAILURE);
>  	}
>  
> -	fd = open(path, O_WRONLY | (force ? 0 : O_EXCL));
> +	fd = open(path, O_RDWR | (force ? 0 : O_EXCL));
>  	if (fd < 0)
>  		err(EXIT_FAILURE, _("cannot open %s"), path);
>  
> @@ -217,6 +252,21 @@ int main(int argc, char **argv)
>  		errx(EXIT_FAILURE, _("%s: length %" PRIu64 " is not aligned "
>  			 "to sector size %i"), path, range[1], secsize);
>  
> +	 /* Check for existing signatures on the device */
> +	if ((ret = device_empty(fd, path)) == 0) {

 What about:

 switch (probe_device(fd, path)) {
 case 0: /* signature detected */
> +		/*
> +		 * Only require force in interactive mode to avoid
> +		 * breaking existing scripts
> +		 */
> +		if (!force && isatty(STDIN_FILENO)) {
> +			errx(EXIT_FAILURE,
> +			     _("This is destructive operation, data will " \
> +			       "be lost! Use the -f option to override."));
> +		}
> +		warnx(_("Operation forced, data will be lost!"));
        break;

 case 1: /* no signature */
        break;

 default: /* error */
> +		err(EXIT_FAILURE, _("failed to probe the device"));
        break;
 }


I think it's more readable ;-)

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] blkdiscard: Refuse to proceed if signatures are found
  2020-06-18 10:06 ` Karel Zak
@ 2020-06-18 10:33   ` Lukas Czerner
  0 siblings, 0 replies; 3+ messages in thread
From: Lukas Czerner @ 2020-06-18 10:33 UTC (permalink / raw)
  To: Karel Zak; +Cc: util-linux

On Thu, Jun 18, 2020 at 12:06:26PM +0200, Karel Zak wrote:
> On Thu, Jun 18, 2020 at 11:29:16AM +0200, Lukas Czerner wrote:
> > With this commit blkdiscard will check for existing signatures on the
> > device and refuse to continue if any are found unless the operation is
> > forced with the -f option.
> 
> Good idea.
> 
> > +/*
> > + * Check existing signature on the open fd
> > + * Returns	0  if no signature was found
> > + * 		1  if a signature was found
> 
> this is not true, 0 means detected, 1 not found
> 
> > + * 		<0 on error
> > + */
> > +static int device_empty(int fd, char *path)
> 
> This is difficult for to read, at first glance it seems according
> to function name that 1 means "yes, it's empty".
>       
> Maybe rename it to probe_device().
> 
> > +{
> > +	const char *type;
> > +	blkid_probe pr = NULL;
> > +	int ret = -1;
> > +
> > +	pr = blkid_new_probe();
> > +	if (!pr || blkid_probe_set_device(pr, fd, 0, 0))
> > +		return ret;
> > +
> > +	blkid_probe_enable_superblocks(pr, TRUE);
> > +	blkid_probe_enable_partitions(pr, TRUE);
> > +
> > +	ret = blkid_do_fullprobe(pr);
> > +	if (ret)
> > +		return ret;
> 
> yes, blkid_do_fullprobe() returns: 0 on success, 1 if nothing is detected or -1 on case of error.
> 
> > +
> > +	if (!blkid_probe_lookup_value(pr, "TYPE", &type, NULL)) {
> > +		warnx("%s contains existing file system (%s).",path ,type);
> > +	} else if (!blkid_probe_lookup_value(pr, "PTTYPE", &type, NULL)) {
> > +		warnx("%s contains existing partition (%s).",path ,type);
> > +	} else {
> > +		warnx("%s contains existing signature.", path);
> > +	}
> > +
> > +	blkid_free_probe(pr);
> > +	return ret;
> 
> This is always 0.
> 
> > +}
> >  
> >  int main(int argc, char **argv)
> >  {
> >  	char *path;
> > -	int c, fd, verbose = 0, secsize, force = 0;
> > +	int c, fd, ret, verbose = 0, secsize, force = 0;
> >  	uint64_t end, blksize, step, range[2], stats[2];
> >  	struct stat sb;
> >  	struct timeval now, last;
> > @@ -184,7 +219,7 @@ int main(int argc, char **argv)
> >  		errtryhelp(EXIT_FAILURE);
> >  	}
> >  
> > -	fd = open(path, O_WRONLY | (force ? 0 : O_EXCL));
> > +	fd = open(path, O_RDWR | (force ? 0 : O_EXCL));
> >  	if (fd < 0)
> >  		err(EXIT_FAILURE, _("cannot open %s"), path);
> >  
> > @@ -217,6 +252,21 @@ int main(int argc, char **argv)
> >  		errx(EXIT_FAILURE, _("%s: length %" PRIu64 " is not aligned "
> >  			 "to sector size %i"), path, range[1], secsize);
> >  
> > +	 /* Check for existing signatures on the device */
> > +	if ((ret = device_empty(fd, path)) == 0) {
> 
>  What about:
> 
>  switch (probe_device(fd, path)) {
>  case 0: /* signature detected */
> > +		/*
> > +		 * Only require force in interactive mode to avoid
> > +		 * breaking existing scripts
> > +		 */
> > +		if (!force && isatty(STDIN_FILENO)) {
> > +			errx(EXIT_FAILURE,
> > +			     _("This is destructive operation, data will " \
> > +			       "be lost! Use the -f option to override."));
> > +		}
> > +		warnx(_("Operation forced, data will be lost!"));
>         break;
> 
>  case 1: /* no signature */
>         break;
> 
>  default: /* error */
> > +		err(EXIT_FAILURE, _("failed to probe the device"));
>         break;
>  }
> 
> 
> I think it's more readable ;-)

Sure, I can do that. The bad comment definitelly makes it more confusing
that it needed to be :)

if (!device_empty()) was the intention but then I still have to deal
with the error. I'll do the switch change.

-Lukas

> 
>     Karel
> 
> -- 
>  Karel Zak  <kzak@redhat.com>
>  http://karelzak.blogspot.com


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-18  9:29 [PATCH] blkdiscard: Refuse to proceed if signatures are found Lukas Czerner
2020-06-18 10:06 ` Karel Zak
2020-06-18 10:33   ` Lukas Czerner

Util-Linux Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/util-linux/0 util-linux/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 util-linux util-linux/ https://lore.kernel.org/util-linux \
		util-linux@vger.kernel.org
	public-inbox-index util-linux

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.util-linux


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git