Re: [PATCH] new command

[OKUJI Yoshinori]

From: "Eugene A. Doudine" <address@hidden>
Subject: [PATCH] new command
Date: Wed, 10 Jan 2001 04:08:12 +0700 (KRAT)

> The command sintax is:
> 
>   ifconfig [--address=IP] [--mask=MASK] [--gateway=IP] [--tftpserver=IP]
> 
> if used without arguments, it will print current network settings.

  Great!

> Here is the patch, could you add these changes to grub source tree?  

  There are a few points I don't like in your patch. First of all,
you should follow the GNU coding style more strictly. For example,

+int ifconfig (char *arg) {
+  struct {
+    char *name;
+    unsigned long *ptr;
+  } args[]={
+    {"--address=",&arptable[ARP_CLIENT].ipaddr.s_addr},
+    {"--tftpserver=",&arptable[ARP_SERVER].ipaddr.s_addr},
+    {"--gateway=",&arptable[ARP_GATEWAY].ipaddr.s_addr},
+    {"--mask=",&netmask},
+    {NULL,NULL}
+  };

This should be:

int
ifconfig (char *arg)
{
  struct
  {
    char *name;
    unsigned long *ptr;
  }
  args[] =
  {
    {"--address=", &arptable[ARP_CLIENT].ipaddr.s_addr},
    {"--tftpserver=", &arptable[ARP_SERVER].ipaddr.s_addr},
    {"--gateway=", &arptable[ARP_GATEWAY].ipaddr.s_addr},
    {"--mask=", &netmask},
    {NULL, NULL}
  };

Second, the implementation of the function "ifconfig" is not
ideal. Don't have any backend function to handle inputs from the user
directly. The command syntax should be defined (and implemented) in
the file "builtins.c" instead. And, ifconfig should take four strings
which represent IP addresses as its arguments, and it should check if
the network configuration is ready rather than blindly setting
NETWORK_READY to non-zero.

Third, you shouldn't have to add grub_strtok. Use skip_to instead.

And, the last is that the option "--tftpserver" would be better to be
renamed to "--server", as we may add other protocols than TFTP in the
future. Then, you may think why the command to specify a TFTP server
is "tftpserver", but this is just for a historical reason and you
shouldn't care.

Thanks,
Okuji