On Thu, Jan 30, 2025 at 6:01 AM Bitácora de Javier Gutiérrez Chamorro
(Guti) via Freedos-devel <freedos-devel@lists.sourceforge.net> wrote:
>
> And now we have 2.01 too:
>
>
> - Changed temporary file naming convention from ZEROFILL.XXX to ZEROXXXX.FIL 
> in order to support up to 9999 files instead of 999 (Fritz Mueller).
>
> - Added a question at the end of the process to confirm deletion of temporary 
> files  (Fritz Mueller).
>
> - Upgraded to UPX 4.2.4.
>
> - Minor code optimizations and cleanup.


For the next update you make to Zerofill, I have a few suggestions:

(1)
At line 98, you do a test of 'argc' to see if 'main' has command line
arguments. The 'else' for this is at the bottom, in lines 187-201.
This code would be easier to read if you instead did a test for "not
enough command line arguments" at top, and exit if not enough, like
this:

if (argc < 2) {
        cprintf("ZEROFILL [drive:]\r\n\r\n");
        cprintf("ZEROFILL writes zeros on the empty disk space for the
selected drive. It helps\r\n");
        cprintf("virtual machine, and disk compression softwares to
compact the volume, and so\r\n");
        cprintf("on reducing its disk usage.\r\n\r\n");
        cprintf("Examples:\r\n\r\n");
        cprintf("       ZEROFILL C:\r\n");
        cprintf("       Will fill with zeros all available space in
drive C.\r\n\r\n");
        cprintf("More information at:\r\n\r\n");
        cprintf("
http://nikkhokkho.sourceforge.net/static.php?page=ZEROFILL\r\n";);
        cprintf("Press ENTER to continue...");
        getch();
        cprintf("\r\n");
        return 1;
}

..then the rest of 'main' continues normally:

        cDrive = toupper(argv[1][0]);
        cprintf("Filling with zero empty space on drive %c:...\r\n", cDrive);
        lTotalMb = 0;
[..]


Although the program doesn't actually use the arguments after the
first one. The usage really is "zerofill d:" where "d:" is a drive
letter like C: or D: or A:

So I'd recommend 'if (argc != 2)'  instead.


That code brings another suggestion:

(2)
I noticed on line 100 that you get the drive letter as toupper(argv[1][0])

But this needs a few more checks before getting the drive letter. In
one example, the user might have passed '/?' assuming they could get
help, but this will instead get used as "drive" letter '/'. And that
will break your math (currently at line 104) to get the drive by
number, because you use 'cDrive - 'A' + 1' (which works correctly if
argv[1][0] is a letter, because 'toupper(argv[1][0])' will return
values like 'A' and 'B' and 'C' and so on).

So I think you should make a test before you get there to see if the
first argument looks like a drive letter. A simple test looks like
this:

int isdriveletter(const char *s)
{
    if (!isalpha(s[0])) {
        return 0;                      /* not letter, or empty */
    }

    if (s[1] != ':') {
        return 0;                      /* no trailing colon */
    }

    if (s[2] != 0) {
        return 0;                      /* trailing data */
    }

    return 1;                          /* looks ok */
}

That doesn't test if a drive letter is valid (i.e. exists) but will
tell you if a string looks like a drive letter ("A:" is okay, "c:" is
okay, but "/?" will fail .. so will "-h" and "*:" and "[:" .. an empty
string also fails).

I'd insert this test after the 'if (argc != 2) { .. }' block I
suggested above, and before the rest of the 'main' function:

if (!isdriveletter(argv[1])) {
  cprintf("%s not a drive letter\r\n", argv[1]);
  return 1;
}

That way, "zerofill" (no args) will fail before you test the first
arg, guaranteeing that 'argc' is 2, so there is an argv[1] to examine.
Then the rest of your code (starting at current line 100) should be
okay.


_______________________________________________
Freedos-devel mailing list
Freedos-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/freedos-devel

Reply via email to