>To: [EMAIL PROTECTED]
>From: "Arkady V.Belousov" <[EMAIL PROTECTED]>
>Subject: [Freedos-kernel] sys.c issues
>
>Hi!
>
>AIU=As I understand.
>
>>int main(int argc, char **argv)
>> if (argc > 1 && memicmp(argv[1], "CONFIG", 6) == 0)
>> else if (memicmp(argp, "BOOTONLY", 8) == 0 && !bootonly)
>> else if (memicmp(argp, "BOTH", 4) == 0 && !both)
>
>- AIU, there should be stricmp() (or third parameter of memicmp() should be
> increased to include zero byte)?
Could, but its more of, are exact arguments here
really necessary? There is next to no chance of a user
accidently typing the initial part and expecting it
to fail because they [purposely or accidently] tacted
on some additional letters. Wasn't this originally
a stricmp?
>
>> COUNT drive; /* destination drive */
>> drive = toupper(argv[drivearg][0]) - 'A';
>> if (drive < 0 || drive >= 26)
>> {
>> printf("%s: drive %c must be A:..Z:\n", pgm,
>> *argv[(argc == 3 ? 2 : 1)]);
>> exit(1);
>> }
>
>- AIU, should be:
>
> UCOUNT drive; /* destination drive */
>--^
> drive = toupper (*argv[drivearg]) - 'A';
> if (drive > 'Z'-'A')
> {
> printf("%s: drive %c must be A:..Z:\n", pgm, *argv[drivearg]);
>-------------------------------------------------------^^^^^^^^
> exit(1);
> }
>
> but I think, this diagnostic may be ommited completely.
It can help pinpoint problems (such as what argument
one types that it thinks is the drive (there was a bug
at one point regarding this or it showed a bug in format).
But yes, ommitting should not hurt the functionality.
>
>> strncpy(srcPath, argv[srcarg], SYS_MAXPATH - 12);
>> /* leave room for COMMAND.COM\0 */
>> srcPath[SYS_MAXPATH - 13] = '\0';
>
>- AIU, should be:
>
> strncpy(srcPath, argv[srcarg], sizeof srcPath - 12);
> /* leave room for COMMAND.COM\0 */
> srcPath[sizeof srcPath - 12] = '\0';
>-----------------------------^^
There are 13 characters in '\\COMMAND.COM\0', which
there must be room for, so the 13 is correct, but the
comment could be updated to include the separator.
If the slash is there, it shouldn't hurt to overwrite
it with a '\0' and if it is not, we must ensure it
can be appended. In any case, if you have a directory
that requires this chunk, chances are things are
going to fail -- its more of a buffer overflow prevention.
>
>> /* make sure srcPath + "file" is a valid path */
>> slen = strlen(srcPath);
>> if ((srcPath[slen - 1] != ':') &&
>> ((srcPath[slen - 1] != '\\') || (srcPath[slen - 1] != '/')))
>> {
>> srcPath[slen] = '\\';
>> slen++;
>> srcPath[slen] = '\0';
>> }
>
>- bug? AIU, should be:
>
> /* make sure srcPath + "file" is a valid path */
> size_t slen = strlen (srcPath);
> if (slen) {
>----^^^^^^^^^
I think at some point the preconditions meant this
check was not required, but perhaps I'm mistaken and
it was overlooked.
>> if ((srcPath[slen - 1] != ':') &&
>> ((srcPath[slen - 1] != '\\') || (srcPath[slen - 1] !=
> char ch = srcPath [slen - 1];
> if (ch != ':' && ch != '\\' && ch != '/')
>-----------------------------------^^
good catch, I think I originally meant [but screwed up]
if (ch != ':' && !((ch == '\\') || (ch == '/')))
but clearly moving the not into the params (as you did)
makes it easier to follow.
> srcPath [slen] = '\\', srcPath [slen + 1] = '\0';
I'd rather see
{
srcPath[slen] = '\\';
srcPath[slen+1] = '\0';
}
over the use of the comma operator, as it makes it
more clear that a compound statement is executed.
slen++ or [slen+1] makes little difference to me
(whichever produces more efficent code)
> }
>
>> /* Get source drive */
>> if ((strlen(srcPath) > 1) && (srcPath[1] == ':')) /* src specifies drive */
>
>- optimization: if (*srcPath && srcPath [1] == ':')
which is ((strlen(srcPath) > 0) && (srcPath[1] == ':'))
The > 1 is to make clear a check ensuring srcPath[1] is
a valid location to check, but as strlen(srcPath) == 1, means
srcPath[1] is the '\0' character, ok.
>
>> /* Don't try root if src==dst drive or source path given */
>> if ((drive == srcDrive)
>> || (*srcPath
>> && ((srcPath[1] != ':') || ((srcPath[1] == ':') && srcPath[2]))))
>> *rootPath = '\0';
>> else
>> sprintf(rootPath, "%c:\\", 'A' + srcDrive);
>
>- (x || (~x && y)) equal to (x && y), so, there should be:
>
> /* Don't try root if src==dst drive or source path given */
> static char root [] = "\0:\\";
> if (drive != srcDrive &&
> (srcPath [0] == '\0' || /* empty string or */
> (srcPath [1] == ':' && srcPath [2] == '\0'))) /* drive only "A:" */
> root [0] = 'A' + srcDrive;
I'm glad you can figure what's being checked for,
?something about not trying root directory for copies
if a path is given as we may be trying to run sys onto
same drive as source using kernel & command.com contained
within a subdirectory of itself
>
>> if (!copy(drive, srcPath, rootPath, kernel_name))
>> {
>> printf("\n%s: cannot copy \"%s\"\n", pgm, kernel_name);
>---------------^^
>
>- if copy() will print "\n" itself, then first "\n" after copy() call will
> not be required.
>
At some point in the past it was necessary, if it is
no longer needed and adds nothing, then ok.
Jeremy
-------------------------------------------------------
SF.Net is sponsored by: Speed Start Your Linux Apps Now.
Build and deploy apps & Web services for Linux with
a free DVD software kit from IBM. Click Now!
http://ads.osdn.com/?ad_id=1356&alloc_id=3438&op=click
_______________________________________________
Freedos-kernel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/freedos-kernel
