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 <phco...@gmail.com>:
>> 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
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>
>
>
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel-- Regards Vladimir 'φ-coder/phcoder' Serbinenko
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
