Re: [PATCH] New command testspeed

[Bean]

Hi,

Pls check out this one.

2012/4/29 Vladimir 'φ-coder/phcoder' Serbinenko <address@hidden>:
> 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
>
>
>
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Best wishes
Bean

testspeed.txt
Description: Text document