Re: [PATCH] Test command

[Yoshinori K. Okuji]

On Saturday 11 April 2009 07:18:59 phcoder wrote:
> Rediffed. New changelog

This time, I comment on all style problems.

> diff --git a/commands/test.c b/commands/test.c
> index a9c8281..2d8dedd 100644
> --- a/commands/test.c
> +++ b/commands/test.c
> @@ -21,33 +21,385 @@
>  #include <grub/misc.h>
>  #include <grub/mm.h>
>  #include <grub/env.h>
> +#include <grub/fs.h>
> +#include <grub/device.h>
> +#include <grub/file.h>
>  #include <grub/command.h>
>
> +/* A simple implementation for signed numbers*/

Please make a complete sentence by terminating it with a period. Also, put two 
space characters before finishing the comment. This is written in the GNU 
Coding Standards.

> +static int
> +grub_strtosl (char *arg, char **end, int base)
> +{
> +  if (arg[0] == '-')
> +    return -grub_strtoul (arg + 1, end, base);
> +  return grub_strtoul (arg, end, base);
> +}
> +
> +/* Parse a test expression startion from *argn*/

Same here.

> +static int
> +test_parse (char **args, int *argn, int argc)
> +{
> +  int ret = 0, discard = 0, invert = 0;
> +  int file_exists;
> +  struct grub_dirhook_info file_info;
> +
> +  auto void update_val (int val);
> +  auto void get_fileinfo (char *pathname);
> +
> +  /*Take care of discarding and inverting*/

Please add a space after the asterisk.

> +  void update_val (int val)
> +  {
> +    if (!discard)

Please add a space.

> +      ret = invert ? !val : val;

Same here.

> +    invert = discard = 0;
> +  }
> +
> +  /* Check if file exists and fetch its information */

Same here.

> +  void get_fileinfo (char *pathname)
> +  {
> +    char *filename, *path;
> +    char *device_name;
> +    grub_fs_t fs;
> +    grub_device_t dev;
> +
> +    /* A hook for iterating directories */

Same here.

> +    auto int find_file (const char *cur_filename,
> +                       struct grub_dirhook_info info);
> +    int find_file (const char *cur_filename, struct grub_dirhook_info
> info) +    {
> +      if (info.case_insensitive ? !grub_strcasecmp (cur_filename,
> filename) +         :!grub_strcmp (cur_filename, filename))

I prefer to avoid treating the return value of strcmp/strcasecmp/memcmp as 
boolean, because even experts often make mistakes. Please use "== 0" instead.

> +       {
> +         file_info = info;
> +         file_exists = 1;
> +         return 1;
> +       }
> +      return 0;
> +    }
> +
> +
> +    file_exists = 0;
> +    device_name = grub_file_get_device_name (pathname);
> +    dev = grub_device_open (device_name);
> +    if (! dev)
> +      {
> +       grub_free (device_name);
> +       return;
> +      }
> +
> +    fs = grub_fs_probe (dev);
> +    path = grub_strchr (pathname, ')');
> +    if (! path)
> +      path = pathname;
> +    else
> +      path++;
> +
> +    /* Remove trailing / */

Same here.

> +    while (*pathname && pathname[grub_strlen (pathname) - 1] == '/')
> +      pathname[grub_strlen (pathname) - 1] = 0;
> +
> +    /* Split into path and filename*/

Same here.

> +    filename = grub_strrchr (pathname, '/');
> +    if (!filename)

Same here.

> +      {
> +       path = grub_strdup ("/");
> +       filename = pathname;
> +      }
> +    else
> +      {
> +       filename++;
> +       path = grub_strdup (pathname);
> +       path[filename - pathname] = 0;
> +      }
> +
> +    /* It's the whole device*/

Same here.

> +    if (!*pathname)

Same here.

> +      {
> +       file_exists = 1;
> +       grub_memset (&file_info, 0, sizeof (file_info));
> +       /* Root is always a directory */

Same here.

> +       file_info.dir = 1;
> +
> +       /* Fetch writing time */

Same here.

> +       file_info.mtimeset = 0;
> +       if (fs->mtime)
> +         {
> +           if (! fs->mtime (dev, &file_info.mtime))
> +             file_info.mtimeset = 1;
> +           grub_errno = GRUB_ERR_NONE;
> +         }
> +      }
> +    else
> +      (fs->dir) (dev, path, find_file);
> +
> +    grub_device_close (dev);
> +    grub_free (path);
> +    grub_free (device_name);
> +  }
> +
> +  /* Here we have the real parsing */

Same here.

> +  while (*argn < argc)
> +    {
> +      /* First try 3 argument tests */

Ditto.

> +      /* String tests */

Ditto.

> +      if (*argn + 2 < argc && (!grub_strcmp (args[*argn + 1], "=")
> +                              || !grub_strcmp (args[*argn + 1], "==")))

Ditto.

> +       {
> +         update_val (grub_strcmp (args[*argn], args[*argn + 2]) == 0);
> +         (*argn) += 3;

I myself feel that these parentheses are redundant, but I don't know how 
others think. For C programmers, it is well known that * has a very high 
priority.

> +         continue;
> +       }
> +
> +      if (*argn + 2 < argc && !grub_strcmp (args[*argn + 1], "!="))

Ditto.

> +       {
> +         update_val (grub_strcmp (args[*argn], args[*argn + 2]) != 0);
> +         (*argn) += 3;
> +         continue;
> +       }
> +
> +      /* GRUB extension: lexicographical sorting */

Ditto.

> +      if (*argn + 2 < argc && !grub_strcmp (args[*argn + 1], "<"))

Ditto.

> +       {
> +         update_val (grub_strcmp (args[*argn], args[*argn + 2]) < 0);
> +         (*argn) += 3;
> +         continue;
> +       }
> +
> +      if (*argn + 2 < argc && !grub_strcmp (args[*argn + 1], "<="))

Ditto.

> +       {
> +         update_val (grub_strcmp (args[*argn], args[*argn + 2]) <= 0);
> +         (*argn) += 3;
> +         continue;
> +       }
> +
> +      if (*argn + 2 < argc && !grub_strcmp (args[*argn + 1], ">"))

Ditto.

> +       {
> +         update_val (grub_strcmp (args[*argn], args[*argn + 2]) > 0);
> +         (*argn) += 3;
> +         continue;
> +       }
> +
> +      if (*argn + 2 < argc && !grub_strcmp (args[*argn + 1], ">="))

Ditto.

> +       {
> +         update_val (grub_strcmp (args[*argn], args[*argn + 2]) >= 0);
> +         (*argn) += 3;
> +         continue;
> +       }
> +
> +      /* Number tests */

Ditto.

> +      if (*argn + 2 < argc && !grub_strcmp (args[*argn + 1], "-eq"))

Ditto.

> +       {
> +         update_val (grub_strtosl (args[*argn], 0, 0)
> +                     == grub_strtosl (args[*argn + 2], 0, 0));
> +         (*argn) += 3;
> +         continue;
> +       }
> +
> +      if (*argn + 2 < argc && !grub_strcmp (args[*argn + 1], "-ge"))

Ditto.

> +       {
> +         update_val (grub_strtosl (args[*argn], 0, 0)
> +                     >= grub_strtosl (args[*argn + 2], 0, 0));
> +         (*argn) += 3;
> +         continue;
> +       }
> +
> +      if (*argn + 2 < argc && !grub_strcmp (args[*argn + 1], "-gt"))

Ditto.

> +       {
> +         update_val (grub_strtosl (args[*argn], 0, 0)
> +                     > grub_strtosl (args[*argn + 2], 0, 0));
> +         (*argn) += 3;
> +         continue;
> +       }
> +
> +      if (*argn + 2 < argc && !grub_strcmp (args[*argn + 1], "-le"))

Ditto.

> +       {
> +         update_val (grub_strtosl (args[*argn], 0, 0)
> +                     <= grub_strtosl (args[*argn + 2], 0, 0));
> +         (*argn) += 3;
> +         continue;
> +       }
> +
> +      if (*argn + 2 < argc && !grub_strcmp (args[*argn + 1], "-lt"))

Ditto.

> +       {
> +         update_val (grub_strtosl (args[*argn], 0, 0)
> +                     < grub_strtosl (args[*argn + 2], 0, 0));
> +         (*argn) += 3;
> +         continue;
> +       }
> +
> +      if (*argn + 2 < argc && !grub_strcmp (args[*argn + 1], "-ne"))

Ditto.

> +       {
> +         update_val (grub_strtosl (args[*argn], 0, 0)
> +                     != grub_strtosl (args[*argn + 2], 0, 0));
> +         (*argn) += 3;
> +         continue;
> +       }
> +
> +      /* GRUB extension: compare numbers skipping prefixes.
> +        Useful for comparing versions. E.g. vmlinuz-2 -plt vmlinuz-11*/

Ditto.

> +      if (*argn + 2 < argc && (!grub_strcmp (args[*argn + 1], "-pgt")
> +                              || !grub_strcmp (args[*argn + 1], "-plt")))

Ditto.

> +       {
> +         int i;
> +         /* Skip common prefix */

Ditto.

> +         for (i = 0; args[*argn][i] == args[*argn + 2][i] &&
> args[*argn][i]; +              i++);
> +
> +         /*Go the digits back*/

Ditto.

> +         i--;
> +         while (grub_isdigit (args[*argn][i]) && i > 0)
> +           i--;
> +         i++;
> +
> +         if (!grub_strcmp (args[*argn + 1], "-pgt"))

Ditto.

> +           update_val (grub_strtoul (args[*argn] + i, 0, 0)
> +                       > grub_strtoul (args[*argn + 2] + i, 0, 0));
> +         else
> +           update_val (grub_strtoul (args[*argn] + i, 0, 0)
> +                       < grub_strtoul (args[*argn + 2] + i, 0, 0));
> +         (*argn) += 3;
> +         continue;
> +       }
> +
> +      /* -nt and -ot tests. GRUB extension: when doing -?t<bias> bias
> +        will be added to the first mtime */

Ditto.

> +      if (*argn + 2 < argc && (!grub_memcmp (args[*argn + 1], "-nt", 3)
> +                              || !grub_memcmp (args[*argn + 1], "-ot",
> 3))) +       {

Ditto.

Getting tired, so I skip the same criticism from here.

> +         struct grub_dirhook_info file1;
> +         int file1exists;
> +         int bias = 0;
> +
> +         /* Fetch fileinfo */
> +         get_fileinfo (args[*argn]);
> +         file1 = file_info;
> +         file1exists = file_exists;
> +         get_fileinfo (args[*argn + 2]);
> +
> +         if (args[*argn + 1][3])
> +           bias = grub_strtosl (args[*argn + 1] + 3, 0, 0);
> +
> +         if (!grub_memcmp (args[*argn + 1], "-nt", 3))
> +           update_val ((file1exists && ! file_exists)
> +                       || (file1.mtimeset && file_info.mtimeset
> +                           && file1.mtime + bias > file_info.mtime));
> +         else
> +           update_val ((!file1exists && file_exists)
> +                       || (file1.mtimeset && file_info.mtimeset
> +                           && file1.mtime + bias < file_info.mtime));
> +         (*argn) += 3;
> +         continue;
> +       }
> +
> +      /* Two-argument tests */
> +      /* file tests*/
> +      if (*argn + 1 < argc && !grub_strcmp (args[*argn], "-d"))
> +       {
> +         get_fileinfo (args[*argn + 1]);
> +         update_val (file_exists && file_info.dir);
> +         (*argn) += 2;
> +         return ret;
> +       }
> +
> +      if (*argn + 1 < argc && !grub_strcmp (args[*argn], "-e"))
> +       {
> +         get_fileinfo (args[*argn + 1]);
> +         update_val (file_exists);
> +         (*argn) += 2;
> +         return ret;
> +       }
> +
> +      if (*argn + 1 < argc && !grub_strcmp (args[*argn], "-f"))
> +       {
> +         get_fileinfo (args[*argn + 1]);
> +         /* FIXME: check for other types */
> +         update_val (file_exists && !file_info.dir);
> +         (*argn) += 2;
> +         return ret;
> +       }
> +
> +      if (*argn + 1 < argc && !grub_strcmp (args[*argn], "-s"))
> +       {
> +         grub_file_t file;
> +         file = grub_file_open (args[*argn + 1]);
> +         update_val (file && grub_file_size (file));

This is not very safe, because grub_file_size returns grub_off_t which is a 
64-bit unsigned int. By converting it into 32-bit signed int implicitly, the 
result can be zero, even when the size is not zero. So it is better to say 
explicitly, != 0.

> +         if (file)
> +           grub_file_close (file);
> +         grub_errno = GRUB_ERR_NONE;
> +         (*argn) += 2;
> +         return ret;
> +       }
> +
> +      /* string tests */
> +      if (!grub_strcmp (args[*argn], "-n") && *argn + 1 < argc)
> +       {
> +         update_val (args[*argn + 1][0]);
> +
> +         (*argn)+=2;
> +         continue;
> +       }
> +      if (!grub_strcmp (args[*argn], "-z") && *argn + 1 < argc)
> +       {
> +         update_val (!args[*argn + 1][0]);
> +         (*argn)+=2;
> +         continue;
> +       }
> +
> +      /* Special modifiers*/
> +
> +      /* End of expression. return to parent*/
> +      if (!grub_strcmp (args[*argn], ")"))
> +       {
> +         (*argn)++;
> +         return ret;
> +       }
> +      /* Recursively invoke if parenthesis */
> +      if (!grub_strcmp (args[*argn], "("))
> +       {
> +         (*argn)++;
> +         update_val (test_parse (args, argn, argc));
> +         continue;
> +       }
> +
> +      if (!grub_strcmp (args[*argn], "!"))
> +       {
> +         invert = !invert;
> +         (*argn)++;
> +         continue;
> +       }
> +      if (!grub_strcmp (args[*argn], "-a"))
> +       {
> +         /* if current value is 0 second value is to be discarded */
> +         discard = !ret;
> +         (*argn)++;
> +         continue;
> +       }
> +      if (!grub_strcmp (args[*argn], "-o"))
> +       {
> +         /* if current value is 1 second value is to be discarded */
> +         discard = ret;
> +         (*argn)++;
> +         continue;
> +       }
> +
> +      /* To test found. Interpret if as just a string*/
> +      update_val (args[*argn][0]);
> +      (*argn)++;
> +    }
> +  return ret;
> +}
> +
>  static grub_err_t
>  grub_cmd_test (grub_command_t cmd __attribute__ ((unused)),
>                int argc, char **args)
>  {
> -  char *eq;
> -  char *eqis;
> -
> -  /* XXX: No fancy expression evaluation yet.  */
> -
> -  if (argc == 0)
> -    return 0;
> -
> -  eq = grub_strdup (args[0]);
> -  eqis = grub_strchr (eq, '=');
> -  if (! eqis)
> -    return 0;
> -
> -  *eqis = '\0';
> -  eqis++;
> -  /* Check an expression in the form `A=B'.  */
> -  if (grub_strcmp (eq, eqis))
> -    grub_error (GRUB_ERR_TEST_FAILURE, "false");
> -  grub_free (eq);
> -
> -  return grub_errno;
> +  int argn = 0;
> +
> +  if (argc >= 1 && !grub_strcmp (args[argc-1], "]"))
> +    argc--;
> +
> +  return test_parse (args, &argn, argc) ? GRUB_ERR_NONE
> +    : grub_error (GRUB_ERR_TEST_FAILURE, "false");
>  }
>
>  static grub_command_t cmd_1, cmd_2;

Regards,
Okuji