Re: [PATCH] New command testspeed

[Vladimir 'φ-coder/phcoder' Serbinenko]

On 29.04.2012 22:09, Bean wrote:
> Hi,
>
> Pls check out this one.
In terms of decreasing code duplication it doesn't improve at all. The
prefix-chosing code needs to be put into a separate function which would
be used by both instances. Also you need "TRANSLATORS" comments before
every of these short messages, otherwise it's untranslatable. I haven't
checked the rest yet.
> 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
>>
>
>
>
>
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel


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

signature.asc
Description: OpenPGP digital signature