From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1SOaeo-0000dk-Af for mharc-grub-devel@gnu.org; Sun, 29 Apr 2012 16:23:46 -0400 Received: from eggs.gnu.org ([208.118.235.92]:48099) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SOael-0000cu-0b for grub-devel@gnu.org; Sun, 29 Apr 2012 16:23:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SOaei-0002SN-Lv for grub-devel@gnu.org; Sun, 29 Apr 2012 16:23:42 -0400 Received: from mail-wi0-f169.google.com ([209.85.212.169]:64413) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SOaei-0002SD-9o for grub-devel@gnu.org; Sun, 29 Apr 2012 16:23:40 -0400 Received: by wibhm17 with SMTP id hm17so1832663wib.0 for ; Sun, 29 Apr 2012 13:23:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=message-id:date:from:user-agent:mime-version:to:subject:references :in-reply-to:x-enigmail-version:content-type; bh=Zd+V+6epiLBhz9T73VGl3dJMfGbWg3CsHBT/kB0UC2I=; b=zmQZ7NHTmrxE3Ji1c47KpaTKPTK6GxMIODb2adMscv6mRa4Mi3msU9Ql25B2pWeFx6 PKymeZHQG2ZKKy8QZcXpm1quRq5ePZfNfImxfkhpNw+XnnAfRrg5qTPoVfKcSBi85GFz rnB0ue9zd+rG7rukqM/nKdIIwlsGly1AXw7xbj+3uRBGYRyXu3c/hjQSrFAzyIeEEdeZ pWEc6sgvunsff6HwV/DUwCThrmI6gJA+JROROt1+JT1JvykkBgAd8tkhdPI99pzJAQw7 HKUMlzvWEe7LqRPgDzDfOl9tn7ULgyRjqEownEwu+LKUImPFB7e/NLjoet2zLPm2oZwn 6cQA== Received: by 10.180.8.231 with SMTP id u7mr23324007wia.9.1335731018000; Sun, 29 Apr 2012 13:23:38 -0700 (PDT) Received: from debian.x201.phnet (226-72.76-83.cust.bluewin.ch. [83.76.72.226]) by mx.google.com with ESMTPS id e6sm23368809wix.8.2012.04.29.13.23.35 (version=TLSv1/SSLv3 cipher=OTHER); Sun, 29 Apr 2012 13:23:37 -0700 (PDT) Message-ID: <4F9DA343.5000708@gmail.com> Date: Sun, 29 Apr 2012 22:23:31 +0200 From: =?UTF-8?B?VmxhZGltaXIgJ8+GLWNvZGVyL3BoY29kZXInIFNlcmJpbmVua28=?= User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0.3) Gecko/20120329 Icedove/10.0.3 MIME-Version: 1.0 To: grub-devel@gnu.org Subject: Re: [PATCH] New command testspeed References: <4F9D5FE6.9040909@gmail.com> In-Reply-To: X-Enigmail-Version: 1.4.1 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="------------enig75DCF686BCB5E4C0E70135CD" X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.85.212.169 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 29 Apr 2012 20:23:44 -0000 This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig75DCF686BCB5E4C0E70135CD Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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 '=CF=86-coder/phcoder' Serbinenko : >> 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 >>> >>> >>> =3D=3D=3D 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 =3D i386; >>> }; >>> >>> +module =3D { >>> + name =3D testspeed; >>> + common =3D commands/testspeed.c; >>> +}; >>> >>> =3D=3D=3D 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[] =3D >>> + { >>> + {"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 **arg= s) >>> +{ >>> + struct grub_arg_list *state =3D 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 =3D=3D 0) >>> + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("no filename is spe= cified")); >> Please avoid adding strings for translation meaning exactly the same a= s >> an already present string but using another form. >> In this case it should have been "filename expected" >>> + >>> + block_size =3D (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 =3D grub_malloc (block_size); >>> + if (buffer =3D=3D NULL) >>> + return grub_errno; >>> + >>> + file =3D grub_file_open (args[0]); >>> + if (file =3D=3D NULL) >>> + goto quit; >>> + >>> + total_size =3D 0; >>> + start =3D grub_get_time_ms (); >>> + while (1) >>> + { >>> + grub_ssize_t size =3D grub_file_read (file, buffer, block_size= ); >>> + if (size <=3D 0) >>> + break; >>> + total_size +=3D size; >>> + } >>> + end =3D 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 !=3D start) >>> + { >>> + grub_uint64_t q, r; >>> + const char *suffix =3D " KMG"; >>> + >>> + q =3D grub_divmod64(total_size * 1000000, end - start, NULL); >>> + while (q > 1024000 && suffix[1] !=3D 0) >> It should be >=3D >>> + { >>> + q >>=3D 10; >>> + suffix++; >>> + } >>> + >>> + q =3D 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 "=D0=93=D0=B8=D0=91" and not "Gi=D0=91". >> 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 '=CF=86-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 --=20 Regards Vladimir '=CF=86-coder/phcoder' Serbinenko --------------enig75DCF686BCB5E4C0E70135CD Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iF4EAREKAAYFAk+do0MACgkQNak7dOguQgnuAgD/WQiwjQynirxWV9akIjwcCK/2 TaCA8eIGtjrKSIfw2IwA/R1G/HidoXK+DfhDxHEhr6/Jftl3kJzDdhYFz8YorDqL =rf4c -----END PGP SIGNATURE----- --------------enig75DCF686BCB5E4C0E70135CD--