All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] New command testspeed
@ 2012-04-29 15:12 Bean
  2012-04-29 15:36 ` Vladimir 'φ-coder/phcoder' Serbinenko
  0 siblings, 1 reply; 5+ messages in thread
From: Bean @ 2012-04-29 15:12 UTC (permalink / raw)
  To: The development of GRUB 2

[-- Attachment #1: Type: text/plain, Size: 182 bytes --]

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

[-- Attachment #2: testspeed.txt --]
[-- Type: text/plain, Size: 3495 bytes --]

=== 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.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/mm.h>
+#include <grub/file.h>
+#include <grub/time.h>
+#include <grub/misc.h>
+#include <grub/dl.h>
+#include <grub/extcmd.h>
+#include <grub/i18n.h>
+
+GRUB_MOD_LICENSE ("GPLv3+");
+
+#define DEFAULT_BLOCK_SIZE	65536
+
+static const struct grub_arg_option options[] =
+  {
+    {"size", 's', 0, N_("Specify block size."), 0, ARG_TYPE_INT},
+    {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"));
+
+  block_size = (state[0].set) ?
+    grub_strtoul (state[0].arg, 0, 0) : DEFAULT_BLOCK_SIZE;
+
+  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);
+
+  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)
+	{
+	  q >>= 10;
+	  suffix++;
+	}
+
+      q = grub_divmod64(q, 1000, &r);
+      grub_printf_ (N_("Speed: %llu.%03d %cB/s \n"),
+		    (unsigned long long) q, (int) r, suffix[0]);
+    }
+
+ quit:
+  grub_free (buffer);
+
+  return grub_errno;
+}
+
+static grub_extcmd_t cmd;
+\f
+GRUB_MOD_INIT(time)
+{
+  cmd = grub_register_extcmd ("testspeed", grub_cmd_testspeed, 0, N_("[-s SIZE] FILENAME"),
+			      N_("Test file read speed."),
+			      options);
+}
+
+GRUB_MOD_FINI(time)
+{
+  grub_unregister_extcmd (cmd);
+}


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] New command testspeed
  2012-04-29 15:12 [PATCH] New command testspeed Bean
@ 2012-04-29 15:36 ` Vladimir 'φ-coder/phcoder' Serbinenko
  2012-04-29 20:09   ` Bean
  0 siblings, 1 reply; 5+ messages in thread
From: Vladimir 'φ-coder/phcoder' Serbinenko @ 2012-04-29 15:36 UTC (permalink / raw)
  To: grub-devel

[-- Attachment #1: Type: text/plain, Size: 4072 bytes --]

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] New command testspeed
  2012-04-29 15:36 ` Vladimir 'φ-coder/phcoder' Serbinenko
@ 2012-04-29 20:09   ` Bean
  2012-04-29 20:23     ` Vladimir 'φ-coder/phcoder' Serbinenko
  0 siblings, 1 reply; 5+ messages in thread
From: Bean @ 2012-04-29 20:09 UTC (permalink / raw)
  To: The development of GNU GRUB

[-- Attachment #1: Type: text/plain, Size: 4691 bytes --]

Hi,

Pls check out this one.

2012/4/29 Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@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
>



-- 
Best wishes
Bean

[-- Attachment #2: testspeed.txt --]
[-- Type: text/plain, Size: 3939 bytes --]

=== 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 20:04:05 +0000
@@ -0,0 +1,129 @@
+/* testspeed.c - Command to test file read speed  */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2012  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/mm.h>
+#include <grub/file.h>
+#include <grub/time.h>
+#include <grub/misc.h>
+#include <grub/dl.h>
+#include <grub/extcmd.h>
+#include <grub/i18n.h>
+
+GRUB_MOD_LICENSE ("GPLv3+");
+
+#define DEFAULT_BLOCK_SIZE	65536
+
+static const struct grub_arg_option options[] =
+  {
+    {"size", 's', 0, N_("Specify size for each read operation"), 0, ARG_TYPE_INT},
+    {0, 0, 0, 0, 0, 0}
+  };
+
+static const char* grub_human_sizes[] =
+  {N_("B"), N_("KiB"), N_("MiB"), N_("GiB"), N_("TiB")};
+
+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_ssize_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_("filename expected"));
+
+  block_size = (state[0].set) ?
+    grub_strtoul (state[0].arg, 0, 0) : DEFAULT_BLOCK_SIZE;
+
+  if (block_size <= 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid block size"));
+
+  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 B \n"), (unsigned long long) total_size);
+  grub_printf_ (N_("Elapsed time: %d.%03d s \n"), (end - start) / 1000,
+		(end - start) % 1000);
+
+  if (end != start)
+    {
+      grub_uint64_t fraction;
+      grub_uint64_t whole =
+	grub_divmod64 (total_size * 1000ULL, end - start, &fraction);
+      grub_uint64_t speed = whole * 1000ULL;
+      int sp = whole;
+      int units = 0;
+
+      while (sp / 1024)
+	{
+	  speed = (speed + 512) / 1024;
+	  sp /= 1024;
+	  units++;
+	}
+
+      if (units)
+	whole = grub_divmod64 (speed, 1000, &fraction);
+      else
+	fraction = grub_divmod64 (fraction * 1000ULL, end - start, NULL);
+
+      grub_printf_ (N_("Speed: %llu.%03d %s/s \n"),
+		    (unsigned long long) whole, (int) fraction,
+		    grub_human_sizes[units]);
+    }
+
+ quit:
+  grub_free (buffer);
+
+  return grub_errno;
+}
+
+static grub_extcmd_t cmd;
+\f
+GRUB_MOD_INIT(time)
+{
+  cmd = grub_register_extcmd ("testspeed", grub_cmd_testspeed, 0, N_("[-s SIZE] FILENAME"),
+			      N_("Test file read speed."),
+			      options);
+}
+
+GRUB_MOD_FINI(time)
+{
+  grub_unregister_extcmd (cmd);
+}


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] New command testspeed
  2012-04-29 20:09   ` Bean
@ 2012-04-29 20:23     ` Vladimir 'φ-coder/phcoder' Serbinenko
  2012-04-29 20:28       ` Vladimir 'φ-coder/phcoder' Serbinenko
  0 siblings, 1 reply; 5+ messages in thread
From: Vladimir 'φ-coder/phcoder' Serbinenko @ 2012-04-29 20:23 UTC (permalink / raw)
  To: grub-devel

[-- Attachment #1: Type: text/plain, Size: 5258 bytes --]

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 <phcoder@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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] New command testspeed
  2012-04-29 20:23     ` Vladimir 'φ-coder/phcoder' Serbinenko
@ 2012-04-29 20:28       ` Vladimir 'φ-coder/phcoder' Serbinenko
  0 siblings, 0 replies; 5+ messages in thread
From: Vladimir 'φ-coder/phcoder' Serbinenko @ 2012-04-29 20:28 UTC (permalink / raw)
  To: grub-devel

[-- Attachment #1: Type: text/plain, Size: 524 bytes --]

On 29.04.2012 22:23, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> 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.
E.g.
#define GRUB_HUMAN_SIZE_MAX_LEN sizeof ("XXXXXXXXXXXXXXXXXXXX TiB")
void grub_huma_size_make_size (char *res, grub_disk_addr_t sz);

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2012-04-29 20:34 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-29 15:12 [PATCH] New command testspeed Bean
2012-04-29 15:36 ` Vladimir 'φ-coder/phcoder' Serbinenko
2012-04-29 20:09   ` Bean
2012-04-29 20:23     ` Vladimir 'φ-coder/phcoder' Serbinenko
2012-04-29 20:28       ` Vladimir 'φ-coder/phcoder' Serbinenko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.