From: "Yoshinori K. Okuji" <okuji@enbug.org>
To: The development of GRUB 2 <grub-devel@gnu.org>
Subject: Re: [PATCH] Test command
Date: Sat, 11 Apr 2009 19:07:08 +0900 [thread overview]
Message-ID: <200904111907.08977.okuji@enbug.org> (raw)
In-Reply-To: <49DFC5D3.3040002@gmail.com>
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
next prev parent reply other threads:[~2009-04-11 10:07 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-02-13 11:39 Test command phcoder
2009-02-13 11:39 ` [PATCH] " phcoder
2009-04-10 22:18 ` phcoder
2009-04-11 10:07 ` Yoshinori K. Okuji [this message]
2009-04-11 15:11 ` phcoder
2009-04-14 15:55 ` Yoshinori K. Okuji
2009-04-16 13:15 ` phcoder
2009-04-25 12:29 ` Vladimir Serbinenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=200904111907.08977.okuji@enbug.org \
--to=okuji@enbug.org \
--cc=grub-devel@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.