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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel