From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1SOWAn-0008Sl-UI for mharc-grub-devel@gnu.org; Sun, 29 Apr 2012 11:36:29 -0400 Received: from eggs.gnu.org ([208.118.235.92]:34283) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SOWAj-0008SS-Og for grub-devel@gnu.org; Sun, 29 Apr 2012 11:36:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SOWAh-0000qa-OX for grub-devel@gnu.org; Sun, 29 Apr 2012 11:36:25 -0400 Received: from mail-wg0-f49.google.com ([74.125.82.49]:62829) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SOWAh-0000qP-Ci for grub-devel@gnu.org; Sun, 29 Apr 2012 11:36:23 -0400 Received: by wgbds1 with SMTP id ds1so1618216wgb.30 for ; Sun, 29 Apr 2012 08:36:21 -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=GmKYMV3mz6X7tqT/pO8RYieWNLFvUe/erB7x1RTt2bQ=; b=rfXM+24syZ4UKArEngrzlMb79ahG3zyA5NscuLQx5hKb2LrzSl+sENcvYhTc0P2Lde 20KRrsPNsMkUrCORwajbLZiwdchtRl4nexfANg5ATD//qgb5QiQo7FxOF8eZu6fXAI/g 4US1eT3h0SlJXb5t6or1JS5NERXT6tmc9uUoEuW+qt/gkynPAimBkBGBcmET031RFsjJ /DdjL49P8O1oAAPh0buAuTrqm/cey0XxeJlxznbBSjc3HfSvy63cJ9VokLWa5oxI3V8P Ah2DiewGvzeO6Vr7D6gYOSsdb58F7CGT97PdviyyoRnC4IGr8+C2ylFH7CMqZipCAU6t s/2A== Received: by 10.180.100.230 with SMTP id fb6mr12680852wib.3.1335713781313; Sun, 29 Apr 2012 08:36:21 -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 ca3sm21570604wib.6.2012.04.29.08.36.19 (version=TLSv1/SSLv3 cipher=OTHER); Sun, 29 Apr 2012 08:36:20 -0700 (PDT) Message-ID: <4F9D5FE6.9040909@gmail.com> Date: Sun, 29 Apr 2012 17:36:06 +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: In-Reply-To: X-Enigmail-Version: 1.4.1 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="------------enigCF5276D63093D268DE958619" X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 74.125.82.49 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 15:36:27 -0000 This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enigCF5276D63093D268DE958619 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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; > }; > =20 > +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 **args)= > +{ > + 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 speci= fied")); 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 =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) t= otal_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. --=20 Regards Vladimir '=CF=86-coder/phcoder' Serbinenko --------------enigCF5276D63093D268DE958619 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+dX/AACgkQNak7dOguQgkM8QD+Ky+7Q2gQvao5N03foNXgoHBA dln24wnyp4ow4YUWMAMA/iMfxz59tBPKVMkCCFMtell9+/u2evFOMNvIZgsw2Lzh =Jnhc -----END PGP SIGNATURE----- --------------enigCF5276D63093D268DE958619--