[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] New command testspeed
From: |
Bean |
Subject: |
Re: [PATCH] New command testspeed |
Date: |
Mon, 30 Apr 2012 04:09:53 +0800 |
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