[PATCH] fdformat: Add new switches -f/--from, -t/--to, -r/--repair

[PATCH] fdformat: Add new switches -f/--from, -t/--to, -r/--repair

[Jaromir Capik]

[-- Attachment #1: Type: text/plain, Size: 512 bytes --]

Hello everyone.

The attached patch adds support for user configurable
from/to track and a basic repair mode for broken floppies.
It also fixes a recently introduced bug that causes
line breakage when printing the track number.

Please, review and share your feelings / merge.

Thanks in advance,
Jaromir.

--
Jaromir Capik
Red Hat Czech, s.r.o.
Software Engineer / Secondary Arch

Email: jcapik@redhat.com
Web: www.cz.redhat.com
Red Hat Czech s.r.o., Purkynova 99/71, 612 45, Brno, Czech Republic
IC: 27690016 

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fdformat-from-to-repair.patch --]
[-- Type: text/x-patch; name=fdformat-from-to-repair.patch, Size: 10380 bytes --]

From d3543ce591fd34a787176993958237864049fbe0 Mon Sep 17 00:00:00 2001
From: Jaromir Capik <jcapik@redhat.com>
Date: Fri, 25 Jul 2014 17:16:56 +0200
Subject: [PATCH] fdformat: Add new switches -f/--from, -t/--to, -r/--repair

This commit introduces a support for user configurable
from/to track and a basic repair mode for broken floppies.
It also fixes a recently introduced bug that causes
a line breakage when printing the track number.
---
 disk-utils/Makemodule.am |   1 +
 disk-utils/fdformat.8    |  11 ++-
 disk-utils/fdformat.c    | 206 +++++++++++++++++++++++++++++++++--------------
 3 files changed, 157 insertions(+), 61 deletions(-)

diff --git a/disk-utils/Makemodule.am b/disk-utils/Makemodule.am
index c6183f5..995e085 100644
--- a/disk-utils/Makemodule.am
+++ b/disk-utils/Makemodule.am
@@ -110,6 +110,7 @@ if BUILD_FDFORMAT
 usrsbin_exec_PROGRAMS += fdformat
 dist_man_MANS += disk-utils/fdformat.8
 fdformat_SOURCES = disk-utils/fdformat.c
+fdformat_LDADD = $(LDADD) libcommon.la
 endif
 
 if BUILD_BLOCKDEV
diff --git a/disk-utils/fdformat.8 b/disk-utils/fdformat.8
index df4153f..797f50f 100644
--- a/disk-utils/fdformat.8
+++ b/disk-utils/fdformat.8
@@ -1,6 +1,6 @@
 .\" Copyright 1992, 1993 Rickard E. Faith (faith@cs.unc.edu)
 .\" May be distributed under the GNU General Public License
-.TH FDFORMAT 8 "July 2011" "util-linux" "System Administration"
+.TH FDFORMAT 8 "July 2014" "util-linux" "System Administration"
 .SH NAME
 fdformat \- low-level format a floppy disk
 .SH SYNOPSIS
@@ -45,6 +45,15 @@ autodetected earlier.  In this case, use
 to load the disk parameters.
 .SH OPTIONS
 .TP
+\fB\-f\fR, \fB\-\-from\fR \fIN\fR
+Start at the track \fIN\fR (default is 0).
+.TP
+\fB\-t\fR, \fB\-\-to\fR \fIN\fR
+Stop at the track \fIN\fR (default is 0).
+.TP
+\fB\-r\fR, \fB\-\-repair\fR \fIN\fR
+Try to repair tracks failed during the verification (max \fIN\fR retries).
+.TP
 \fB\-n\fR, \fB\-\-no-verify\fR
 Skip the verification that is normally performed after the formatting.
 .TP
diff --git a/disk-utils/fdformat.c b/disk-utils/fdformat.c
index e6ae8e4..c8b2a81 100644
--- a/disk-utils/fdformat.c
+++ b/disk-utils/fdformat.c
@@ -12,6 +12,7 @@
 #include <unistd.h>
 
 #include "c.h"
+#include "strutils.h"
 #include "closestream.h"
 #include "nls.h"
 #include "xalloc.h"
@@ -20,74 +21,119 @@ struct floppy_struct param;
 
 #define SECTOR_SIZE 512
 
-static void format_disk(int ctrl)
+
+static void format_begin(int ctrl)
+{
+	if (ioctl(ctrl, FDFMTBEG, NULL) < 0)
+		err(EXIT_FAILURE, "ioctl: FDFMTBEG");
+}
+
+static void format_end(int ctrl)
+{
+	if (ioctl(ctrl, FDFMTEND, NULL) < 0)
+		err(EXIT_FAILURE, "ioctl: FDFMTEND");
+}
+
+static void format_track_head(int ctrl, unsigned int track, unsigned int head)
 {
 	struct format_descr descr;
-	unsigned int track;
+
+	descr.track = track;
+	descr.head = head;
+	if (ioctl(ctrl, FDFMTTRK, (long) &descr) < 0)
+		err(EXIT_FAILURE, "ioctl: FDFMTTRK");
+}
+
+static void seek_track_head(int ctrl, unsigned int track, unsigned int head)
+{
+	lseek(ctrl, (track * param.head + head) * param.sect * SECTOR_SIZE, SEEK_SET);
+}
+
+static void format_disk(int ctrl, unsigned int track_from, unsigned int track_to)
+{
+	unsigned int track, head;
 
 	printf(_("Formatting ... "));
 	fflush(stdout);
-	if (ioctl(ctrl, FDFMTBEG, NULL) < 0)
-		err(EXIT_FAILURE, "ioctl: FDFMTBEG");
-	for (track = 0; track < param.track; track++) {
-		descr.track = track;
-		descr.head = 0;
-		if (ioctl(ctrl, FDFMTTRK, (long) &descr) < 0)
-			err(EXIT_FAILURE, "ioctl: FDFMTTRK");
-
-		printf("%3ud\b\b\b", track);
-		fflush(stdout);
-		if (param.head == 2) {
-			descr.head = 1;
-			if (ioctl(ctrl, FDFMTTRK, (long)&descr) < 0)
-				err(EXIT_FAILURE, "ioctl: FDFMTTRK");
+
+	format_begin(ctrl);
+
+	for (track = track_from; track <= track_to; track++) {
+		for (head = 0; head < param.head; head++) {
+			printf("%3u/%u\b\b\b\b\b", track, head);
+			fflush(stdout);
+			format_track_head(ctrl, track, head);
 		}
 	}
-	if (ioctl(ctrl, FDFMTEND, NULL) < 0)
-		err(EXIT_FAILURE, "ioctl: FDFMTEND");
+
+	format_end(ctrl);
+
 	printf(_("done\n"));
 }
 
-static void verify_disk(char *name)
+static void verify_disk(int ctrl, unsigned int track_from, unsigned int track_to, int repair)
 {
 	unsigned char *data;
-	unsigned int cyl;
-	int fd, cyl_size, count;
+	unsigned int track, head;
+	int track_size, count, retries_left;
 
-	cyl_size = param.sect * param.head * 512;
-	data = xmalloc(cyl_size);
+	track_size = param.sect * SECTOR_SIZE;
+	data = xmalloc(track_size);
 	printf(_("Verifying ... "));
 	fflush(stdout);
-	if ((fd = open(name, O_RDONLY)) < 0)
-		err(EXIT_FAILURE, _("cannot open %s"), name);
-	for (cyl = 0; cyl < param.track; cyl++) {
-		int read_bytes;
-
-		printf("%u3d\b\b\b", cyl);
-		fflush(stdout);
-		read_bytes = read(fd, data, cyl_size);
-		if (read_bytes != cyl_size) {
-			if (read_bytes < 0)
-				perror(_("Read: "));
-			fprintf(stderr,
-				_("Problem reading cylinder %d,"
-				  " expected %d, read %d\n"),
-				cyl, cyl_size, read_bytes);
-			free(data);
-			exit(EXIT_FAILURE);
-		}
-		for (count = 0; count < cyl_size; count++)
-			if (data[count] != FD_FILL_BYTE) {
-				printf(_("bad data in cyl %d\n"
-					 "Continuing ... "), cyl);
-				fflush(stdout);
+
+	seek_track_head (ctrl, track_from, 0);
+
+	for (track = track_from; track <= track_to; track++) {
+		for (head = 0; head < param.head; head++) {
+			int read_bytes;
+
+			printf("%3u\b\b\b", track);
+			fflush(stdout);
+
+			retries_left = repair;
+			do {
+				read_bytes = read(ctrl, data, track_size);
+				if (read_bytes != track_size) {
+					if (retries_left) {
+						format_begin(ctrl);
+						format_track_head(ctrl, track, head);
+						format_end(ctrl);
+						seek_track_head (ctrl, track, head);
+						retries_left--;
+						if (retries_left) continue;
+					}
+					if (read_bytes < 0)
+						perror(_("Read: "));
+					fprintf(stderr,
+						_("Problem reading track/head %u/%u,"
+						  " expected %d, read %d\n"),
+						track, head, track_size, read_bytes);
+					free(data);
+					exit(EXIT_FAILURE);
+				}
+				for (count = 0; count < track_size; count++)
+					if (data[count] != FD_FILL_BYTE) {
+						if (retries_left) {
+							format_begin(ctrl);
+							format_track_head(ctrl, track, head);
+							format_end(ctrl);
+							seek_track_head (ctrl, track, head);
+							retries_left--;
+							if (retries_left) continue;
+						}
+						printf(_("bad data in track/head %u/%u\n"
+							 "Continuing ... "), track, head);
+						fflush(stdout);
+						break;
+					}
 				break;
-			}
+			} while (retries_left);
+		}
 	}
+
 	free(data);
 	printf(_("done\n"));
-	if (close(fd) < 0)
-		err(EXIT_FAILURE, "close");
 }
 
 static void __attribute__ ((__noreturn__)) usage(FILE * out)
@@ -96,9 +142,13 @@ static void __attribute__ ((__noreturn__)) usage(FILE * out)
 		program_invocation_short_name);
 
 	fprintf(out, _("\nOptions:\n"
-		       " -n, --no-verify  disable the verification after the format\n"
-		       " -V, --version    output version information and exit\n"
-		       " -h, --help       display this help and exit\n\n"));
+		       " -f, --from <N>    start at the track N (default 0)\n"
+		       " -t, --to <N>      stop at the track N\n"
+		       " -r, --repair <N>  try to repair tracks failed during\n"
+		       "                   the verification (max N retries)\n"
+		       " -n, --no-verify   disable the verification after the format\n"
+		       " -V, --version     output version information and exit\n"
+		       " -h, --help        display this help and exit\n\n"));
 
 	exit(out == stderr ? EXIT_FAILURE : EXIT_SUCCESS);
 }
@@ -108,22 +158,45 @@ int main(int argc, char **argv)
 	int ch;
 	int ctrl;
 	int verify = 1;
+	int repair = 0;
+	int track_from = 0;
+	int track_to = -1;
+	unsigned long arg;
 	struct stat st;
 
 	static const struct option longopts[] = {
+		{"from", required_argument, NULL, 'f'},
+		{"to", required_argument, NULL, 't'},
+		{"repair", required_argument, NULL, 'r'},
 		{"no-verify", no_argument, NULL, 'n'},
 		{"version", no_argument, NULL, 'V'},
 		{"help", no_argument, NULL, 'h'},
 		{NULL, 0, NULL, 0}
 	};
 
+
 	setlocale(LC_ALL, "");
 	bindtextdomain(PACKAGE, LOCALEDIR);
 	textdomain(PACKAGE);
 	atexit(close_stdout);
 
-	while ((ch = getopt_long(argc, argv, "nVh", longopts, NULL)) != -1)
+	while ((ch = getopt_long(argc, argv, "f:t:r:nVh", longopts, NULL)) != -1)
 		switch (ch) {
+		case 'f':
+			arg = strtoul_or_err(optarg, _("invalid argument - from"));
+			if (arg > INT_MAX) err(EXIT_FAILURE, _("invalid argument - from"));
+			track_from = arg;
+			break;
+		case 't':
+			arg = strtoul_or_err(optarg, _("invalid argument - to"));
+			if (arg > INT_MAX) err(EXIT_FAILURE, _("invalid argument - to"));
+			track_to = arg;
+			break;
+		case 'r':
+			arg = strtoul_or_err(optarg, _("invalid argument - repair"));
+			if (arg > INT_MAX) err(EXIT_FAILURE, _("invalid argument - repair"));
+			repair = arg;
+			break;
 		case 'n':
 			verify = 0;
 			break;
@@ -149,20 +222,33 @@ int main(int argc, char **argv)
 	if (access(argv[0], W_OK) < 0)
 		err(EXIT_FAILURE, _("cannot access file %s"), argv[0]);
 
-	ctrl = open(argv[0], O_WRONLY);
+	ctrl = open(argv[0], O_RDWR);
 	if (ctrl < 0)
 		err(EXIT_FAILURE, _("cannot open %s"), argv[0]);
 	if (ioctl(ctrl, FDGETPRM, (long)&param) < 0)
 		err(EXIT_FAILURE, _("Could not determine current format type"));
 
 	printf(_("%s-sided, %d tracks, %d sec/track. Total capacity %d kB.\n"),
-	       (param.head == 2) ? _("Double") : _("Single"),
-	       param.track, param.sect, param.size >> 1);
-	format_disk(ctrl);
-	if (close_fd(ctrl) != 0)
-		err(EXIT_FAILURE, _("write failed"));
+		(param.head == 2) ? _("Double") : _("Single"),
+		param.track, param.sect, param.size >> 1);
+
+	if (track_to == -1)
+		track_to = param.track - 1;
+
+	if ((unsigned int)track_from > param.track - 1)
+		err(EXIT_FAILURE, _("'from' is out of allowed range\n"));
+	if ((unsigned int)track_to > param.track - 1)
+		err(EXIT_FAILURE, _("'to' is out of allowed range\n"));
+	if (track_from > track_to)
+		err(EXIT_FAILURE, _("'from' is higher than 'to'\n"));
+
+	format_disk(ctrl, track_from, track_to);
 
 	if (verify)
-		verify_disk(argv[0]);
+		verify_disk(ctrl, track_from, track_to, repair);
+
+	if (close_fd(ctrl) != 0)
+		err(EXIT_FAILURE, _("close failed"));
+
 	return EXIT_SUCCESS;
 }
-- 
1.9.3

Re: [PATCH] fdformat: Add new switches -f/--from, -t/--to, -r/--repair

[Sami Kerola]

On 25 July 2014 16:29, Jaromir Capik <jcapik@redhat.com> wrote:

Hi Jaromir,

> It also fixes a recently introduced bug that causes
> line breakage when printing the track number.

Do you mean commit 18b1fcb9cc62f4dd26eccd1c35f612ac6db1cd9b or some other
change?  It would be nice to know details of what exactly was wrong, and
how the fix is fixing it.  The change you sent is rather long, and at
least I cannot see where is the correction.  That said splitting the
change to smaller chunks would be nice.


> diff --git a/disk-utils/Makemodule.am b/disk-utils/Makemodule.am
> index c6183f5..995e085 100644
> --- a/disk-utils/Makemodule.am
> +++ b/disk-utils/Makemodule.am
> @@ -110,6 +110,7 @@ if BUILD_FDFORMAT
>  usrsbin_exec_PROGRAMS += fdformat
>  dist_man_MANS += disk-utils/fdformat.8
>  fdformat_SOURCES = disk-utils/fdformat.c
> +fdformat_LDADD = $(LDADD) libcommon.la
>  endif
>
>  if BUILD_BLOCKDEV
> diff --git a/disk-utils/fdformat.8 b/disk-utils/fdformat.8
> index df4153f..797f50f 100644
> --- a/disk-utils/fdformat.8
> +++ b/disk-utils/fdformat.8
> @@ -1,6 +1,6 @@
>  .\" Copyright 1992, 1993 Rickard E. Faith (faith@cs.unc.edu)
>  .\" May be distributed under the GNU General Public License
> -.TH FDFORMAT 8 "July 2011" "util-linux" "System Administration"
> +.TH FDFORMAT 8 "July 2014" "util-linux" "System Administration"
>  .SH NAME
>  fdformat \- low-level format a floppy disk
>  .SH SYNOPSIS
> @@ -45,6 +45,15 @@ autodetected earlier.  In this case, use
>  to load the disk parameters.
>  .SH OPTIONS
>  .TP
> +\fB\-f\fR, \fB\-\-from\fR \fIN\fR
> +Start at the track \fIN\fR (default is 0).
> +.TP
> +\fB\-t\fR, \fB\-\-to\fR \fIN\fR
> +Stop at the track \fIN\fR (default is 0).
> +.TP
> +\fB\-r\fR, \fB\-\-repair\fR \fIN\fR
> +Try to repair tracks failed during the verification (max \fIN\fR retries).
> +.TP
>  \fB\-n\fR, \fB\-\-no-verify\fR
>  Skip the verification that is normally performed after the formatting.
>  .TP
> diff --git a/disk-utils/fdformat.c b/disk-utils/fdformat.c
> index e6ae8e4..c8b2a81 100644
> --- a/disk-utils/fdformat.c
> +++ b/disk-utils/fdformat.c
> @@ -12,6 +12,7 @@
>  #include <unistd.h>
>
>  #include "c.h"
> +#include "strutils.h"
>  #include "closestream.h"
>  #include "nls.h"
>  #include "xalloc.h"
> @@ -20,74 +21,119 @@ struct floppy_struct param;
>
>  #define SECTOR_SIZE 512
>
> -static void format_disk(int ctrl)
> +
> +static void format_begin(int ctrl)
> +{
> + if (ioctl(ctrl, FDFMTBEG, NULL) < 0)
> + err(EXIT_FAILURE, "ioctl: FDFMTBEG");
> +}
> +
> +static void format_end(int ctrl)
> +{
> + if (ioctl(ctrl, FDFMTEND, NULL) < 0)
> + err(EXIT_FAILURE, "ioctl: FDFMTEND");
> +}
> +

The format_begin() and format_end() are not doing much, are they really
needed?

> +static void format_track_head(int ctrl, unsigned int track, unsigned int head)
>  {
>   struct format_descr descr;
> - unsigned int track;
> +
> + descr.track = track;
> + descr.head = head;
> + if (ioctl(ctrl, FDFMTTRK, (long) &descr) < 0)
> + err(EXIT_FAILURE, "ioctl: FDFMTTRK");
> +}
> +
> +static void seek_track_head(int ctrl, unsigned int track, unsigned int head)
> +{
> + lseek(ctrl, (track * param.head + head) * param.sect * SECTOR_SIZE, SEEK_SET);
> +}
> +

Would it be possible to more  'struct floppy_struct param;' out of the
global scope?  That would make it a bit less mysterious where param.head
came from.

> +static void format_disk(int ctrl, unsigned int track_from, unsigned int track_to)
> +{
> + unsigned int track, head;

The track & head could be replaced with

struct format_descr descr;

When the two helper variables are gone the format_track_head() turns to a
single if (ioctl...  statement, and I'd claim it is unnecessary.

>   printf(_("Formatting ... "));
>   fflush(stdout);
> - if (ioctl(ctrl, FDFMTBEG, NULL) < 0)
> - err(EXIT_FAILURE, "ioctl: FDFMTBEG");
> - for (track = 0; track < param.track; track++) {
> - descr.track = track;
> - descr.head = 0;
> - if (ioctl(ctrl, FDFMTTRK, (long) &descr) < 0)
> - err(EXIT_FAILURE, "ioctl: FDFMTTRK");
> -
> - printf("%3ud\b\b\b", track);
> - fflush(stdout);
> - if (param.head == 2) {
> - descr.head = 1;
> - if (ioctl(ctrl, FDFMTTRK, (long)&descr) < 0)
> - err(EXIT_FAILURE, "ioctl: FDFMTTRK");
> +
> + format_begin(ctrl);
> +
> + for (track = track_from; track <= track_to; track++) {
> + for (head = 0; head < param.head; head++) {
> + printf("%3u/%u\b\b\b\b\b", track, head);
> + fflush(stdout);
> + format_track_head(ctrl, track, head);
>   }
>   }
> - if (ioctl(ctrl, FDFMTEND, NULL) < 0)
> - err(EXIT_FAILURE, "ioctl: FDFMTEND");
> +
> + format_end(ctrl);
> +
>   printf(_("done\n"));
>  }
>
> -static void verify_disk(char *name)
> +static void verify_disk(int ctrl, unsigned int track_from, unsigned int track_to, int repair)
>  {
>   unsigned char *data;
> - unsigned int cyl;
> - int fd, cyl_size, count;
> + unsigned int track, head;
> + int track_size, count, retries_left;
>
> - cyl_size = param.sect * param.head * 512;
> - data = xmalloc(cyl_size);
> + track_size = param.sect * SECTOR_SIZE;
> + data = xmalloc(track_size);
>   printf(_("Verifying ... "));
>   fflush(stdout);
> - if ((fd = open(name, O_RDONLY)) < 0)
> - err(EXIT_FAILURE, _("cannot open %s"), name);
> - for (cyl = 0; cyl < param.track; cyl++) {
> - int read_bytes;
> -
> - printf("%u3d\b\b\b", cyl);
> - fflush(stdout);
> - read_bytes = read(fd, data, cyl_size);
> - if (read_bytes != cyl_size) {
> - if (read_bytes < 0)
> - perror(_("Read: "));
> - fprintf(stderr,
> - _("Problem reading cylinder %d,"
> -  " expected %d, read %d\n"),
> - cyl, cyl_size, read_bytes);
> - free(data);
> - exit(EXIT_FAILURE);
> - }
> - for (count = 0; count < cyl_size; count++)
> - if (data[count] != FD_FILL_BYTE) {
> - printf(_("bad data in cyl %d\n"
> - "Continuing ... "), cyl);
> - fflush(stdout);
> +
> + seek_track_head (ctrl, track_from, 0);
> +
> + for (track = track_from; track <= track_to; track++) {
> + for (head = 0; head < param.head; head++) {
> + int read_bytes;
> +
> + printf("%3u\b\b\b", track);
> + fflush(stdout);
> +
> + retries_left = repair;
> + do {
> + read_bytes = read(ctrl, data, track_size);
> + if (read_bytes != track_size) {
> + if (retries_left) {
> + format_begin(ctrl);
> + format_track_head(ctrl, track, head);
> + format_end(ctrl);
> + seek_track_head (ctrl, track, head);
> + retries_left--;

Will the retries work if IO error happens?  There are calls to err() all
over, so am I thinking wrong the program will exit rather than re-attempt
until retries end.

> + if (retries_left) continue;

New line & indent step missing.

> + }
> + if (read_bytes < 0)
> + perror(_("Read: "));
> + fprintf(stderr,
> + _("Problem reading track/head %u/%u,"
> +  " expected %d, read %d\n"),
> + track, head, track_size, read_bytes);
> + free(data);
> + exit(EXIT_FAILURE);
> + }
> + for (count = 0; count < track_size; count++)
> + if (data[count] != FD_FILL_BYTE) {
> + if (retries_left) {
> + format_begin(ctrl);
> + format_track_head(ctrl, track, head);
> + format_end(ctrl);
> + seek_track_head (ctrl, track, head);
> + retries_left--;
> + if (retries_left) continue;

Same as previous two comments.

> + }
> + printf(_("bad data in track/head %u/%u\n"
> + "Continuing ... "), track, head);
> + fflush(stdout);
> + break;
> + }
>   break;
> - }
> + } while (retries_left);
> + }
>   }
> +
>   free(data);
>   printf(_("done\n"));
> - if (close(fd) < 0)
> - err(EXIT_FAILURE, "close");
>  }
>
>  static void __attribute__ ((__noreturn__)) usage(FILE * out)
> @@ -96,9 +142,13 @@ static void __attribute__ ((__noreturn__)) usage(FILE * out)
>   program_invocation_short_name);
>
>   fprintf(out, _("\nOptions:\n"
> -       " -n, --no-verify  disable the verification after the format\n"
> -       " -V, --version    output version information and exit\n"
> -       " -h, --help       display this help and exit\n\n"));
> +       " -f, --from <N>    start at the track N (default 0)\n"
> +       " -t, --to <N>      stop at the track N\n"
> +       " -r, --repair <N>  try to repair tracks failed during\n"
> +       "                   the verification (max N retries)\n"
> +       " -n, --no-verify   disable the verification after the format\n"
> +       " -V, --version     output version information and exit\n"
> +       " -h, --help        display this help and exit\n\n"));
>
>   exit(out == stderr ? EXIT_FAILURE : EXIT_SUCCESS);
>  }
> @@ -108,22 +158,45 @@ int main(int argc, char **argv)
>   int ch;
>   int ctrl;
>   int verify = 1;
> + int repair = 0;
> + int track_from = 0;
> + int track_to = -1;

In floppy_struct track is unsigned int.  It would be best to not mix
types unnecessarily.

> + unsigned long arg;
>   struct stat st;
>
>   static const struct option longopts[] = {
> + {"from", required_argument, NULL, 'f'},
> + {"to", required_argument, NULL, 't'},
> + {"repair", required_argument, NULL, 'r'},
>   {"no-verify", no_argument, NULL, 'n'},
>   {"version", no_argument, NULL, 'V'},
>   {"help", no_argument, NULL, 'h'},
>   {NULL, 0, NULL, 0}
>   };
>
> +
>   setlocale(LC_ALL, "");
>   bindtextdomain(PACKAGE, LOCALEDIR);
>   textdomain(PACKAGE);
>   atexit(close_stdout);
>
> - while ((ch = getopt_long(argc, argv, "nVh", longopts, NULL)) != -1)
> + while ((ch = getopt_long(argc, argv, "f:t:r:nVh", longopts, NULL)) != -1)
>   switch (ch) {
> + case 'f':
> + arg = strtoul_or_err(optarg, _("invalid argument - from"));
> + if (arg > INT_MAX) err(EXIT_FAILURE, _("invalid argument - from"));

Use strtou32_or_err() and remove INT_MAX check.

> + track_from = arg;
> + break;
> + case 't':
> + arg = strtoul_or_err(optarg, _("invalid argument - to"));
> + if (arg > INT_MAX) err(EXIT_FAILURE, _("invalid argument - to"));
> + track_to = arg;
> + break;
> + case 'r':
> + arg = strtoul_or_err(optarg, _("invalid argument - repair"));
> + if (arg > INT_MAX) err(EXIT_FAILURE, _("invalid argument - repair"));
> + repair = arg;
> + break;
>   case 'n':
>   verify = 0;
>   break;
> @@ -149,20 +222,33 @@ int main(int argc, char **argv)
>   if (access(argv[0], W_OK) < 0)
>   err(EXIT_FAILURE, _("cannot access file %s"), argv[0]);
>
> - ctrl = open(argv[0], O_WRONLY);
> + ctrl = open(argv[0], O_RDWR);
>   if (ctrl < 0)
>   err(EXIT_FAILURE, _("cannot open %s"), argv[0]);
>   if (ioctl(ctrl, FDGETPRM, (long)&param) < 0)
>   err(EXIT_FAILURE, _("Could not determine current format type"));
>
>   printf(_("%s-sided, %d tracks, %d sec/track. Total capacity %d kB.\n"),
> -       (param.head == 2) ? _("Double") : _("Single"),
> -       param.track, param.sect, param.size >> 1);
> - format_disk(ctrl);
> - if (close_fd(ctrl) != 0)
> - err(EXIT_FAILURE, _("write failed"));
> + (param.head == 2) ? _("Double") : _("Single"),
> + param.track, param.sect, param.size >> 1);
> +
> + if (track_to == -1)
> + track_to = param.track - 1;
> +

Adding a 'user_defined_tracks' variable seems better approach, than using
contents of the variable to figure out whether it was set or not.

> + if ((unsigned int)track_from > param.track - 1)
> + err(EXIT_FAILURE, _("'from' is out of allowed range\n"));
> + if ((unsigned int)track_to > param.track - 1)
> + err(EXIT_FAILURE, _("'to' is out of allowed range\n"));
> + if (track_from > track_to)
> + err(EXIT_FAILURE, _("'from' is higher than 'to'\n"));

The err() adds new line to print out, so the messages above will print
unexpected empty line.

<nitpick>
Writing the if cases in smaller greater order makes code quicker to  read.

if (smaller_value < greater_value)
err()
</nitpick>

> +
> + format_disk(ctrl, track_from, track_to);
>
>   if (verify)
> - verify_disk(argv[0]);
> + verify_disk(ctrl, track_from, track_to, repair);
> +
> + if (close_fd(ctrl) != 0)
> + err(EXIT_FAILURE, _("close failed"));
> +
>   return EXIT_SUCCESS;
>  }


-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH] fdformat: Add new switches -f/--from, -t/--to, -r/--repair

[Jaromir Capik]

> Hi Jaromir,

Hi Sami.


> > It also fixes a recently introduced bug that causes
> > line breakage when printing the track number.
> 
> Do you mean commit 18b1fcb9cc62f4dd26eccd1c35f612ac6db1cd9b or some other
> change? 

Yes, exactly this commit.


> It would be nice to know details of what exactly was wrong, and
> how the fix is fixing it.

Ok ... so:
1.) The following hunk changes %3d to invalid %3ud and that caused
an extra character 'd' printed with each track change and as there
are only 3 backspaces, the whole string was slowly moving to the
right instead of staying at the same place. 

-		printf("%3d\b\b\b", track);
+		printf("%3ud\b\b\b", track);

Note, that 'u' is NOT a modifier, that needs to be prepended to 'd'.
The 'd' specifier needs to be replaced with 'u' instead.

2.) The following hunk changes %3d to invalid %u3d and that caused
extra characters '3d' appended to each track number.

-		printf("%3d\b\b\b", cyl);
+		printf("%u3d\b\b\b", cyl);

And after crossing the 10th track the final result was '103d'
where 3 backspace chars were unable to return to the first
characters and that produced something like:

111111111122222222223333333333444444473d

Note that '3' is a width modifier and always needs to be prepended
to the 'u' type specifier. So, the position matters:


>  The change you sent is rather long, and at
> least I cannot see where is the correction.  That said splitting the
> change to smaller chunks would be nice.

It has no sense to split that as I modified the code to support
heads and the format string had to be changed to a different one.


> > +static void format_begin(int ctrl)
> > +{
> > + if (ioctl(ctrl, FDFMTBEG, NULL) < 0)
> > + err(EXIT_FAILURE, "ioctl: FDFMTBEG");
> > +}
> > +
> > +static void format_end(int ctrl)
> > +{
> > + if (ioctl(ctrl, FDFMTEND, NULL) < 0)
> > + err(EXIT_FAILURE, "ioctl: FDFMTEND");
> > +}
> > +
> 
> The format_begin() and format_end() are not doing much, are they really
> needed?

They increase the code abstraction and readability.
There's a condition checking the return state and the code
is reused 3 times. Nobody can notice those wasted microseconds
when the whole format takes minutes. I really believe it is worthy.
We could possibly change them to inline functions, but it is really
not necessary in this case.


> > +static void format_track_head(int ctrl, unsigned int track, unsigned int
> > head)
> >  {
> >   struct format_descr descr;
> > - unsigned int track;
> > +
> > + descr.track = track;
> > + descr.head = head;
> > + if (ioctl(ctrl, FDFMTTRK, (long) &descr) < 0)
> > + err(EXIT_FAILURE, "ioctl: FDFMTTRK");
> > +}
> > +
> > +static void seek_track_head(int ctrl, unsigned int track, unsigned int
> > head)
> > +{
> > + lseek(ctrl, (track * param.head + head) * param.sect * SECTOR_SIZE,
> > SEEK_SET);
> > +}
> > +
> 
> Would it be possible to more  'struct floppy_struct param;' out of the
> global scope?  That would make it a bit less mysterious where param.head
> came from.

At first I tested exactly this variant, then I rewrote it to 2 separate
variables as (according to my opinion) the code looked better readable
and understandable. But I'm not against changing this as both variants
make sense for me.


> > +static void format_disk(int ctrl, unsigned int track_from, unsigned int
> > track_to)
> > +{
> > + unsigned int track, head;
> 
> The track & head could be replaced with
> 
> struct format_descr descr;
> 
> When the two helper variables are gone the format_track_head() turns to a
> single if (ioctl...  statement, and I'd claim it is unnecessary.

... hopefully answered above.


> > + retries_left = repair;
> > + do {
> > + read_bytes = read(ctrl, data, track_size);
> > + if (read_bytes != track_size) {
> > + if (retries_left) {
> > + format_begin(ctrl);
> > + format_track_head(ctrl, track, head);
> > + format_end(ctrl);
> > + seek_track_head (ctrl, track, head);
> > + retries_left--;
> 
> Will the retries work if IO error happens?  There are calls to err() all
> over, so am I thinking wrong the program will exit rather than re-attempt
> until retries end.

What IO error do you mean? IO errors are handled by the following condition:

  if (read_bytes != track_size) {

I tested the code with several broken floppies and it does exactly what
I wanted to achieve. You probably missed that "continue" call when at least
one retry is left. If the track is unreadable, it decreases the retry counter,
re-format the track, re-seeks the track for verification and jumps back
to the beginning of the track verification loop.

> 
> > + if (retries_left) continue;
> 
> New line & indent step missing.

I usually don't indent if well readable and other lines are longer,
but no problem with that. Can be changed.


> > @@ -108,22 +158,45 @@ int main(int argc, char **argv)
> >   int ch;
> >   int ctrl;
> >   int verify = 1;
> > + int repair = 0;
> > + int track_from = 0;
> > + int track_to = -1;
> 
> In floppy_struct track is unsigned int.  It would be best to not mix
> types unnecessarily.

The negative value is used as a flag here. -1 means it is not set
by the user and this way I reused the same variable.
I correctly retype them when needed.


> Use strtou32_or_err() and remove INT_MAX check.

Ok, had no idea it exists. Looked at few other tools and copied that.
This is a good candidate for a change.


> > + if (track_to == -1)
> > + track_to = param.track - 1;
> > +
> 
> Adding a 'user_defined_tracks' variable seems better approach, than using
> contents of the variable to figure out whether it was set or not.


This is specific to the '-t/--to' switch only. Both switches can
be used independently. You can use just one of them or both together.
The variable would have to be called 'user_defined_track_to'.


> > + if ((unsigned int)track_from > param.track - 1)
> > + err(EXIT_FAILURE, _("'from' is out of allowed range\n"));
> > + if ((unsigned int)track_to > param.track - 1)
> > + err(EXIT_FAILURE, _("'to' is out of allowed range\n"));
> > + if (track_from > track_to)
> > + err(EXIT_FAILURE, _("'from' is higher than 'to'\n"));
> 
> The err() adds new line to print out, so the messages above will print
> unexpected empty line.

This can be completely removed after the change to strtou32_or_err(), right?


Please, let me know.

Thanks,
Jaromir.

--
Jaromir Capik
Red Hat Czech, s.r.o.
Software Engineer / Secondary Arch

Email: jcapik@redhat.com
Web: www.cz.redhat.com
Red Hat Czech s.r.o., Purkynova 99/71, 612 45, Brno, Czech Republic
IC: 27690016 

Re: [PATCH] fdformat: Add new switches -f/--from, -t/--to, -r/--repair

[Karel Zak]

On Mon, Jul 28, 2014 at 10:38:39AM -0400, Jaromir Capik wrote:
> > Do you mean commit 18b1fcb9cc62f4dd26eccd1c35f612ac6db1cd9b or some other
> > change? 
... 
> -		printf("%3d\b\b\b", track);
> +		printf("%3ud\b\b\b", track);

 This is obvious...

> >  The change you sent is rather long, and at
> > least I cannot see where is the correction.  That said splitting the
> > change to smaller chunks would be nice.
> 
> It has no sense to split that as I modified the code to support
> heads and the format string had to be changed to a different one.

 Perfect is the enemy of good. It's no so huge (all the original code 
 has ~160 lines).

> > The format_begin() and format_end() are not doing much, are they really
> > needed?
> 
> They increase the code abstraction and readability.

 +1

> > > + if (retries_left) continue;
> > 
> > New line & indent step missing.
> 
> I usually don't indent if well readable and other lines are longer,
> but no problem with that. Can be changed.

 Please, change it.

> > Use strtou32_or_err() and remove INT_MAX check.
> 
> Ok, had no idea it exists. Looked at few other tools and copied that.
> This is a good candidate for a change.

 Yep.

> > > + if ((unsigned int)track_from > param.track - 1)
> > > + err(EXIT_FAILURE, _("'from' is out of allowed range\n"));
> > > + if ((unsigned int)track_to > param.track - 1)
> > > + err(EXIT_FAILURE, _("'to' is out of allowed range\n"));
> > > + if (track_from > track_to)
> > > + err(EXIT_FAILURE, _("'from' is higher than 'to'\n"));
> > 
> > The err() adds new line to print out, so the messages above will print
> > unexpected empty line.
> 
> This can be completely removed after the change to strtou32_or_err(), right?

 I guess you still want to check user's input against the hardware, so check
 if track_to is smaller than param.track is correct thing. Right?

 Anyway, \n is unnecessary, errx() does not print error based on
 errno, err() follows errno. 


 Thank, it's nice to see we have someone to test fdformat!

    Karel

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

Re: [PATCH] fdformat: Add new switches -f/--from, -t/--to, -r/--repair

[Jaromir Capik]

Ahoj Karle.

> > > > + if ((unsigned int)track_from > param.track - 1)
> > > > + err(EXIT_FAILURE, _("'from' is out of allowed range\n"));
> > > > + if ((unsigned int)track_to > param.track - 1)
> > > > + err(EXIT_FAILURE, _("'to' is out of allowed range\n"));
> > > > + if (track_from > track_to)
> > > > + err(EXIT_FAILURE, _("'from' is higher than 'to'\n"));
> > > 
> > > The err() adds new line to print out, so the messages above will print
> > > unexpected empty line.
> > 
> > This can be completely removed after the change to strtou32_or_err(),
> > right?
> 
>  I guess you still want to check user's input against the hardware, so check
>  if track_to is smaller than param.track is correct thing. Right?

The comparison with the param.track is done elsewhere as we don't know
the disk parameters during the argument parsing yet.
We get the parameters after finishing the argument parsing.


>  Anyway, \n is unnecessary, errx() does not print error based on
>  errno, err() follows errno.

Sure, this was more of a copy'n'paste error.


>  Thank, it's nice to see we have someone to test fdformat!

Np. Can test any time.
Going to rework the patch.

Thx,
Jaromir.

--
Jaromir Capik
Red Hat Czech, s.r.o.
Software Engineer / Secondary Arch

Email: jcapik@redhat.com
Web: www.cz.redhat.com
Red Hat Czech s.r.o., Purkynova 99/71, 612 45, Brno, Czech Republic
IC: 27690016

Re: [PATCH] fdformat: Add new switches -f/--from, -t/--to, -r/--repair

[Jaromir Capik]

> > > > > + if ((unsigned int)track_from > param.track - 1)
> > > > > + err(EXIT_FAILURE, _("'from' is out of allowed range\n"));
> > > > > + if ((unsigned int)track_to > param.track - 1)
> > > > > + err(EXIT_FAILURE, _("'to' is out of allowed range\n"));
> > > > > + if (track_from > track_to)
> > > > > + err(EXIT_FAILURE, _("'from' is higher than 'to'\n"));
> > > > 
> > > > The err() adds new line to print out, so the messages above will print
> > > > unexpected empty line.
> > > 
> > > This can be completely removed after the change to strtou32_or_err(),
> > > right?
> > 
> >  I guess you still want to check user's input against the hardware, so
> >  check
> >  if track_to is smaller than param.track is correct thing. Right?
> 
> The comparison with the param.track is done elsewhere as we don't know
> the disk parameters during the argument parsing yet.
> We get the parameters after finishing the argument parsing.

Sorry, I overlooked and thought you're writing about the argument
parsing. Of course, this needs to be preserved.

I've reworked the patch, but I'll send it after doing few tests at home.

J.

Re: [PATCH] fdformat: Add new switches -f/--from, -t/--to, -r/--repair

[Jaromir Capik]

[-- Attachment #1: Type: text/plain, Size: 301 bytes --]

Hello guys. 

The reworked patch is attached. 

Please, review once more. 

Thanks, 
Jaromir. 

--
Jaromir Capik
Red Hat Czech, s.r.o.
Software Engineer / Secondary Arch

Email: jcapik@redhat.com
Web: www.cz.redhat.com
Red Hat Czech s.r.o., Purkynova 99/71, 612 45, Brno, Czech Republic
IC: 27690016 

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fdformat-from-to-repair.patch --]
[-- Type: text/x-patch; name=fdformat-from-to-repair.patch, Size: 10564 bytes --]

From 9b9954ec709cdb5ba44b142aa96dac9fdc09cac2 Mon Sep 17 00:00:00 2001
From: Jaromir Capik <jcapik@redhat.com>
Date: Mon, 28 Jul 2014 20:47:09 +0200
Subject: [PATCH] fdformat: Add new switches -f/--from, -t/--to, -r/--repair

This commit introduces a support for user configurable
from/to track and a basic repair mode for broken floppies.
It also fixes a recently introduced bug that causes
a line breakage when printing the track number.
---
 disk-utils/Makemodule.am |   1 +
 disk-utils/fdformat.8    |  11 ++-
 disk-utils/fdformat.c    | 210 ++++++++++++++++++++++++++++++++---------------
 3 files changed, 157 insertions(+), 65 deletions(-)

diff --git a/disk-utils/Makemodule.am b/disk-utils/Makemodule.am
index c6183f5..995e085 100644
--- a/disk-utils/Makemodule.am
+++ b/disk-utils/Makemodule.am
@@ -110,6 +110,7 @@ if BUILD_FDFORMAT
 usrsbin_exec_PROGRAMS += fdformat
 dist_man_MANS += disk-utils/fdformat.8
 fdformat_SOURCES = disk-utils/fdformat.c
+fdformat_LDADD = $(LDADD) libcommon.la
 endif
 
 if BUILD_BLOCKDEV
diff --git a/disk-utils/fdformat.8 b/disk-utils/fdformat.8
index df4153f..797f50f 100644
--- a/disk-utils/fdformat.8
+++ b/disk-utils/fdformat.8
@@ -1,6 +1,6 @@
 .\" Copyright 1992, 1993 Rickard E. Faith (faith@cs.unc.edu)
 .\" May be distributed under the GNU General Public License
-.TH FDFORMAT 8 "July 2011" "util-linux" "System Administration"
+.TH FDFORMAT 8 "July 2014" "util-linux" "System Administration"
 .SH NAME
 fdformat \- low-level format a floppy disk
 .SH SYNOPSIS
@@ -45,6 +45,15 @@ autodetected earlier.  In this case, use
 to load the disk parameters.
 .SH OPTIONS
 .TP
+\fB\-f\fR, \fB\-\-from\fR \fIN\fR
+Start at the track \fIN\fR (default is 0).
+.TP
+\fB\-t\fR, \fB\-\-to\fR \fIN\fR
+Stop at the track \fIN\fR (default is 0).
+.TP
+\fB\-r\fR, \fB\-\-repair\fR \fIN\fR
+Try to repair tracks failed during the verification (max \fIN\fR retries).
+.TP
 \fB\-n\fR, \fB\-\-no-verify\fR
 Skip the verification that is normally performed after the formatting.
 .TP
diff --git a/disk-utils/fdformat.c b/disk-utils/fdformat.c
index e6ae8e4..dc98de5 100644
--- a/disk-utils/fdformat.c
+++ b/disk-utils/fdformat.c
@@ -12,82 +12,129 @@
 #include <unistd.h>
 
 #include "c.h"
+#include "strutils.h"
 #include "closestream.h"
 #include "nls.h"
 #include "xalloc.h"
 
+#define SECTOR_SIZE 512
+
 struct floppy_struct param;
 
-#define SECTOR_SIZE 512
 
-static void format_disk(int ctrl)
+static void format_begin(int ctrl)
 {
-	struct format_descr descr;
-	unsigned int track;
+	if (ioctl(ctrl, FDFMTBEG, NULL) < 0)
+		err(EXIT_FAILURE, "ioctl: FDFMTBEG");
+}
+
+static void format_end(int ctrl)
+{
+	if (ioctl(ctrl, FDFMTEND, NULL) < 0)
+		err(EXIT_FAILURE, "ioctl: FDFMTEND");
+}
+
+static void format_track_head(int ctrl, struct format_descr *descr)
+{
+	if (ioctl(ctrl, FDFMTTRK, (long) descr) < 0)
+		err(EXIT_FAILURE, "ioctl: FDFMTTRK");
+}
+
+static void seek_track_head(int ctrl, struct format_descr *descr)
+{
+	lseek(ctrl, (descr->track * param.head + descr->head) * param.sect * SECTOR_SIZE, SEEK_SET);
+}
+
+static void format_disk(int ctrl, unsigned int track_from, unsigned int track_to)
+{
+	struct format_descr current;
 
 	printf(_("Formatting ... "));
 	fflush(stdout);
-	if (ioctl(ctrl, FDFMTBEG, NULL) < 0)
-		err(EXIT_FAILURE, "ioctl: FDFMTBEG");
-	for (track = 0; track < param.track; track++) {
-		descr.track = track;
-		descr.head = 0;
-		if (ioctl(ctrl, FDFMTTRK, (long) &descr) < 0)
-			err(EXIT_FAILURE, "ioctl: FDFMTTRK");
-
-		printf("%3ud\b\b\b", track);
-		fflush(stdout);
-		if (param.head == 2) {
-			descr.head = 1;
-			if (ioctl(ctrl, FDFMTTRK, (long)&descr) < 0)
-				err(EXIT_FAILURE, "ioctl: FDFMTTRK");
+
+	format_begin(ctrl);
+
+	for (current.track = track_from; current.track <= track_to; current.track++) {
+		for (current.head = 0; current.head < param.head; current.head++) {
+			printf("%3u/%u\b\b\b\b\b", current.track, current.head);
+			fflush(stdout);
+			format_track_head(ctrl, &current);
 		}
 	}
-	if (ioctl(ctrl, FDFMTEND, NULL) < 0)
-		err(EXIT_FAILURE, "ioctl: FDFMTEND");
+
+	format_end(ctrl);
+
 	printf(_("done\n"));
 }
 
-static void verify_disk(char *name)
+static void verify_disk(int ctrl, unsigned int track_from, unsigned int track_to, unsigned int repair)
 {
 	unsigned char *data;
-	unsigned int cyl;
-	int fd, cyl_size, count;
+	struct format_descr current;
+	int track_size, count;
+	unsigned int retries_left;
 
-	cyl_size = param.sect * param.head * 512;
-	data = xmalloc(cyl_size);
+	track_size = param.sect * SECTOR_SIZE;
+	data = xmalloc(track_size);
 	printf(_("Verifying ... "));
 	fflush(stdout);
-	if ((fd = open(name, O_RDONLY)) < 0)
-		err(EXIT_FAILURE, _("cannot open %s"), name);
-	for (cyl = 0; cyl < param.track; cyl++) {
-		int read_bytes;
-
-		printf("%u3d\b\b\b", cyl);
-		fflush(stdout);
-		read_bytes = read(fd, data, cyl_size);
-		if (read_bytes != cyl_size) {
-			if (read_bytes < 0)
-				perror(_("Read: "));
-			fprintf(stderr,
-				_("Problem reading cylinder %d,"
-				  " expected %d, read %d\n"),
-				cyl, cyl_size, read_bytes);
-			free(data);
-			exit(EXIT_FAILURE);
-		}
-		for (count = 0; count < cyl_size; count++)
-			if (data[count] != FD_FILL_BYTE) {
-				printf(_("bad data in cyl %d\n"
-					 "Continuing ... "), cyl);
-				fflush(stdout);
+
+	current.track = track_from;
+	current.head = 0;
+	seek_track_head (ctrl, &current);
+
+	for (current.track = track_from; current.track <= track_to; current.track++) {
+		for (current.head = 0; current.head < param.head; current.head++) {
+			int read_bytes;
+
+			printf("%3u\b\b\b", current.track);
+			fflush(stdout);
+
+			retries_left = repair;
+			do {
+				read_bytes = read(ctrl, data, track_size);
+				if (read_bytes != track_size) {
+					if (retries_left) {
+						format_begin(ctrl);
+						format_track_head(ctrl, &current);
+						format_end(ctrl);
+						seek_track_head (ctrl, &current);
+						retries_left--;
+						if (retries_left)
+							continue;
+					}
+					if (read_bytes < 0)
+						perror(_("Read: "));
+					fprintf(stderr,
+						_("Problem reading track/head %u/%u,"
+						  " expected %d, read %d\n"),
+						current.track, current.head, track_size, read_bytes);
+					free(data);
+					exit(EXIT_FAILURE);
+				}
+				for (count = 0; count < track_size; count++)
+					if (data[count] != FD_FILL_BYTE) {
+						if (retries_left) {
+							format_begin(ctrl);
+							format_track_head(ctrl, &current);
+							format_end(ctrl);
+							seek_track_head (ctrl, &current);
+							retries_left--;
+							if (retries_left)
+								continue;
+						}
+						printf(_("bad data in track/head %u/%u\n"
+							 "Continuing ... "), current.track, current.head);
+						fflush(stdout);
+						break;
+					}
 				break;
-			}
+			} while (retries_left);
+		}
 	}
+
 	free(data);
 	printf(_("done\n"));
-	if (close(fd) < 0)
-		err(EXIT_FAILURE, "close");
 }
 
 static void __attribute__ ((__noreturn__)) usage(FILE * out)
@@ -96,9 +143,13 @@ static void __attribute__ ((__noreturn__)) usage(FILE * out)
 		program_invocation_short_name);
 
 	fprintf(out, _("\nOptions:\n"
-		       " -n, --no-verify  disable the verification after the format\n"
-		       " -V, --version    output version information and exit\n"
-		       " -h, --help       display this help and exit\n\n"));
+		       " -f, --from <N>    start at the track N (default 0)\n"
+		       " -t, --to <N>      stop at the track N\n"
+		       " -r, --repair <N>  try to repair tracks failed during\n"
+		       "                   the verification (max N retries)\n"
+		       " -n, --no-verify   disable the verification after the format\n"
+		       " -V, --version     output version information and exit\n"
+		       " -h, --help        display this help and exit\n\n"));
 
 	exit(out == stderr ? EXIT_FAILURE : EXIT_SUCCESS);
 }
@@ -108,22 +159,40 @@ int main(int argc, char **argv)
 	int ch;
 	int ctrl;
 	int verify = 1;
+	unsigned int repair = 0;
+	unsigned int track_from = 0;
+	unsigned int track_to = 0;
+	int has_user_defined_track_to = 0;
 	struct stat st;
 
 	static const struct option longopts[] = {
+		{"from", required_argument, NULL, 'f'},
+		{"to", required_argument, NULL, 't'},
+		{"repair", required_argument, NULL, 'r'},
 		{"no-verify", no_argument, NULL, 'n'},
 		{"version", no_argument, NULL, 'V'},
 		{"help", no_argument, NULL, 'h'},
 		{NULL, 0, NULL, 0}
 	};
 
+
 	setlocale(LC_ALL, "");
 	bindtextdomain(PACKAGE, LOCALEDIR);
 	textdomain(PACKAGE);
 	atexit(close_stdout);
 
-	while ((ch = getopt_long(argc, argv, "nVh", longopts, NULL)) != -1)
+	while ((ch = getopt_long(argc, argv, "f:t:r:nVh", longopts, NULL)) != -1)
 		switch (ch) {
+		case 'f':
+			track_from = strtou32_or_err(optarg, _("invalid argument - from"));
+			break;
+		case 't':
+			has_user_defined_track_to = 1;
+			track_to = strtou32_or_err(optarg, _("invalid argument - to"));
+			break;
+		case 'r':
+			repair = strtou32_or_err(optarg, _("invalid argument - repair"));
+			break;
 		case 'n':
 			verify = 0;
 			break;
@@ -149,20 +218,33 @@ int main(int argc, char **argv)
 	if (access(argv[0], W_OK) < 0)
 		err(EXIT_FAILURE, _("cannot access file %s"), argv[0]);
 
-	ctrl = open(argv[0], O_WRONLY);
+	ctrl = open(argv[0], O_RDWR);
 	if (ctrl < 0)
 		err(EXIT_FAILURE, _("cannot open %s"), argv[0]);
-	if (ioctl(ctrl, FDGETPRM, (long)&param) < 0)
-		err(EXIT_FAILURE, _("Could not determine current format type"));
+	if (ioctl(ctrl, FDGETPRM, (long) &param) < 0)
+		err(EXIT_FAILURE, _("could not determine current format type"));
 
 	printf(_("%s-sided, %d tracks, %d sec/track. Total capacity %d kB.\n"),
-	       (param.head == 2) ? _("Double") : _("Single"),
-	       param.track, param.sect, param.size >> 1);
-	format_disk(ctrl);
-	if (close_fd(ctrl) != 0)
-		err(EXIT_FAILURE, _("write failed"));
+		(param.head == 2) ? _("Double") : _("Single"),
+		param.track, param.sect, param.size >> 1);
+
+	if (!has_user_defined_track_to)
+		track_to = param.track - 1;
+
+	if (track_from >= param.track)
+		err(EXIT_FAILURE, _("user defined start track exceeds the medium specific maximum"));
+	if (track_to >= param.track)
+		err(EXIT_FAILURE, _("user defined end track exceeds the medium specific maximum"));
+	if (track_from > track_to)
+		err(EXIT_FAILURE, _("user defined start track exceeds the user defined end track"));
+
+	format_disk(ctrl, track_from, track_to);
 
 	if (verify)
-		verify_disk(argv[0]);
+		verify_disk(ctrl, track_from, track_to, repair);
+
+	if (close_fd(ctrl) != 0)
+		err(EXIT_FAILURE, _("close failed"));
+
 	return EXIT_SUCCESS;
 }
-- 
1.9.3

Re: [PATCH] fdformat: Add new switches -f/--from, -t/--to, -r/--repair

[Karel Zak]

On Tue, Jul 29, 2014 at 07:32:03AM -0400, Jaromir Capik wrote:
>  disk-utils/Makemodule.am |   1 +
>  disk-utils/fdformat.8    |  11 ++-
>  disk-utils/fdformat.c    | 210 ++++++++++++++++++++++++++++++++---------------
>  3 files changed, 157 insertions(+), 65 deletions(-)

 Applied, thanks!

    Karel

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

Re: [PATCH] fdformat: Add new switches -f/--from, -t/--to, -r/--repair

[Jaromir Capik]

[-- Attachment #1: Type: text/plain, Size: 1042 bytes --]

Hello everyone.

I often re-check my patches few weeks after the push
and that allowed me to find a copy'n'paste issue
in the manual.

The fix is attached.

Thanks for pushing.

Regards,
Jaromir.

--
Jaromir Capik
Red Hat Czech, s.r.o.
Software Engineer / Secondary Arch

Email: jcapik@redhat.com
Web: www.cz.redhat.com
Red Hat Czech s.r.o., Purkynova 99/71, 612 45, Brno, Czech Republic
IC: 27690016 


----- Original Message -----
> From: "Jaromir Capik" <jcapik@redhat.com>
> To: "Karel Zak" <kzak@redhat.com>
> Cc: kerolasa@gmail.com, "util-linux" <util-linux@vger.kernel.org>
> Sent: Tuesday, July 29, 2014 1:32:03 PM
> Subject: Re: [PATCH] fdformat: Add new switches -f/--from, -t/--to, -r/--repair
> 
> Hello guys.
> 
> The reworked patch is attached.
> 
> Please, review once more.
> 
> Thanks,
> Jaromir.
> 
> --
> Jaromir Capik
> Red Hat Czech, s.r.o.
> Software Engineer / Secondary Arch
> 
> Email: jcapik@redhat.com
> Web: www.cz.redhat.com
> Red Hat Czech s.r.o., Purkynova 99/71, 612 45, Brno, Czech Republic
> IC: 27690016
> 

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fdformat-man-copy-n-paste-bug.patch --]
[-- Type: text/x-patch; name=fdformat-man-copy-n-paste-bug.patch, Size: 823 bytes --]

From 540b94d786e2c602b4b99da06cae3c6f81fa1315 Mon Sep 17 00:00:00 2001
From: Jaromir Capik <jcapik@redhat.com>
Date: Mon, 8 Sep 2014 19:42:10 +0200
Subject: [PATCH] fdformat: fixing copy'n'paste issue in the manual

The end track default is media specific, not zero.
---
 disk-utils/fdformat.8 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/disk-utils/fdformat.8 b/disk-utils/fdformat.8
index 797f50f..ae1e530 100644
--- a/disk-utils/fdformat.8
+++ b/disk-utils/fdformat.8
@@ -49,7 +49,7 @@ to load the disk parameters.
 Start at the track \fIN\fR (default is 0).
 .TP
 \fB\-t\fR, \fB\-\-to\fR \fIN\fR
-Stop at the track \fIN\fR (default is 0).
+Stop at the track \fIN\fR.
 .TP
 \fB\-r\fR, \fB\-\-repair\fR \fIN\fR
 Try to repair tracks failed during the verification (max \fIN\fR retries).
-- 
1.9.3

Re: [PATCH] fdformat: Add new switches -f/--from, -t/--to, -r/--repair

[Karel Zak]

On Mon, Sep 08, 2014 at 01:50:02PM -0400, Jaromir Capik wrote:
> The fix is attached.

 Applied, thanks.

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