Re: [PATCH] New command testspeed

[Vladimir 'φ-coder/phcoder' Serbinenko]

On 29.04.2012 17:12, Bean wrote:
> Hi,
>
> This patch add a new command testspeed which read a file and then
> print the speed. It's quite useful in debugging the efficiency of fs
> or network drivers.
>
> -- Best wishes Bean
>
> testspeed.txt
>
>
> === modified file 'grub-core/Makefile.core.def'
> --- grub-core/Makefile.core.def       2012-04-01 19:35:18 +0000
> +++ grub-core/Makefile.core.def       2012-04-29 12:10:27 +0000
> @@ -1840,3 +1840,7 @@
>    enable = i386;
>  };
>  
> +module = {
> +  name = testspeed;
> +  common = commands/testspeed.c;
> +};
>
> === added file 'grub-core/commands/testspeed.c'
> --- grub-core/commands/testspeed.c    1970-01-01 00:00:00 +0000
> +++ grub-core/commands/testspeed.c    2012-04-29 15:10:24 +0000
> @@ -0,0 +1,114 @@
> +/* testspeed.c - Command to test file read speed  */
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2011  Free Software Foundation, Inc.
We're in 2012, not 2011.
> + *
> +
> +static const struct grub_arg_option options[] =
> +  {
> +    {"size", 's', 0, N_("Specify block size."), 0, ARG_TYPE_INT},
The name of the option is confusing. Someone may think it's about
limiting total size.
> +    {0, 0, 0, 0, 0, 0}
> +  };
> +
> +static grub_err_t
> +grub_cmd_testspeed (grub_extcmd_context_t ctxt, int argc, char **args)
> +{
> +  struct grub_arg_list *state = ctxt->state;
> +  grub_uint32_t start;
> +  grub_uint32_t end;
> +  grub_size_t block_size;
> +  grub_disk_addr_t total_size;
> +  char *buffer;
> +  grub_file_t file;
> +
> +  if (argc == 0)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("no filename is 
> specified"));
Please avoid adding strings for translation meaning exactly the same as
an already present string but using another form.
In this case it should have been "filename expected"
> +
> +  block_size = (state[0].set) ?
> +    grub_strtoul (state[0].arg, 0, 0) : DEFAULT_BLOCK_SIZE;
You forget to check the validity of block_size. (in particular 0 is
invalid, overflowing numbers probably as well)
> +
> +  buffer = grub_malloc (block_size);
> +  if (buffer == NULL)
> +    return grub_errno;
> +
> +  file = grub_file_open (args[0]);
> +  if (file == NULL)
> +    goto quit;
> +
> +  total_size = 0;
> +  start = grub_get_time_ms ();
> +  while (1)
> +    {
> +      grub_ssize_t size = grub_file_read (file, buffer, block_size);
> +      if (size <= 0)
> +     break;
> +      total_size += size;
> +    }
> +  end = grub_get_time_ms ();
> +  grub_file_close (file);
> +
> +  grub_printf_ (N_("File size: %llu bytes \n"), (unsigned long long) 
> total_size);
> +  grub_printf_ (N_("Elapsed time: %d.%03d seconds \n"), (end - start) / 1000,
> +             (end - start) % 1000);
Even in English these sentences are numerically incorrect. E.g
"File size: 1 bytes"
In other languages it gets worse since the form may depend on trailing
digit. Please use units abbreviations as they are invariant.
> +
> +  if (end != start)
> +    {
> +      grub_uint64_t q, r;
> +      const char *suffix = " KMG";
> +
> +      q = grub_divmod64(total_size * 1000000, end - start, NULL);
> +      while (q > 1024000 && suffix[1] != 0)
It should be >=
> +     {
> +       q >>= 10;
> +       suffix++;
> +     }
> +
> +      q = grub_divmod64(q, 1000, &r);
This whole algorithm uses too much divisions. Moreover a better
algorithm is already available in ls.c. Please avoid duplicating code
and use already present algorithm.
> +      grub_printf_ (N_("Speed: %llu.%03d %cB/s \n"),
> +                 (unsigned long long) q, (int) r, suffix[0]);
It's wrong since you work with binary prefixes and so it should be KiB
and not KB. Also suffixes need to be translatable as well. E.g. in
Russian one would use "ГиБ" and not "GiБ".
While old code isn't properly localisable yet (i.a. hdparm code is a
mess) and it's part of ongoing effort, all new code has to be fully
localisable, other than the limitations documented in manual or
developer manual.


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko

signature.asc
Description: OpenPGP digital signature