> 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@xxxxxxxxxx
Web: www.cz.redhat.com
Red Hat Czech s.r.o., Purkynova 99/71, 612 45, Brno, Czech Republic
IC: 27690016
--
To unsubscribe from this list: send the line "unsubscribe util-linux" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
