A few other code suggestions:
I prefer putting "fail" statements first, so the program can safely
exit early. This also keeps the "failure" message next to the test,
which I find easier to read.
For example, on line 107, you do a 'malloc' then test if you have a
value in line 109, but the "failure" message happens after all that
code, on lines 177-180:
107 pcBuffer = malloc(SHRT_MAX + 1);
108 /* If we have allocated memory */
109 if (pcBuffer != NULL)
110 {
[...]
176 }
177 else
178 {
179 cprintf("Error. Cannot allocate memory.\r\n");
180 }
I think it's better to write it like this:
pcBuffer = malloc(SHRT_MAX + 1);
if (pcBuffer == NULL)
{
cprintf("Error. Cannot allocate memory.\r\n");
return 1;
}
..then continue with the "success" code (currently lines 110-176).
Similarly, you do a test for disk free space on line 104, but the
"failure" message happens on lines 182-185:
104 if (_dos_getdiskfree_ext(cDrive - 'A' + 1,
&udtDiskFreeExt) == 0)
105 {
[...]
181 }
182 else
183 {
184 cprintf("Error. Cannot get free disk space for
%c:.\r\n", cDrive);
185 }
I think it's more clear to write it the other way:
if (_dos_getdiskfree_ext(cDrive - 'A' + 1, &udtDiskFreeExt) != 0) {
cprintf("Error. Cannot get free disk space for %c:.\r\n", cDrive);
return 1;
}
..then continue with the "success" code (currently lines 105-181).
Writing code this way is basically a series of tests, followed by the
code that you can safely run, like this sample "do-nothing" program:
int main()
{
if (argc != 2) {
puts("bad command line usage");
return 1;
}
if (failtest1) {
puts("argv[1] is not a drive letter");
return 2;
}
if (failtest2) {
puts("drive does not exist");
return 3;
}
if (failtest3) {
puts("not enough free disk space");
return 4;
}
if (failtest4) {
puts("not enough memory");
return 5;
}
/* things should be ok from here */
...
return 0;
}
On Thu, Jan 30, 2025 at 9:08 AM Jim Hall <[email protected]> wrote:
>
> On Thu, Jan 30, 2025 at 6:01 AM Bitácora de Javier Gutiérrez Chamorro
> (Guti) via Freedos-devel <[email protected]> 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
[email protected]
https://lists.sourceforge.net/lists/listinfo/freedos-devel
