From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1KZXxa-0003JL-Me for mharc-grub-devel@gnu.org; Sat, 30 Aug 2008 17:26:18 -0400 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KZXxY-0003JG-Nk for grub-devel@gnu.org; Sat, 30 Aug 2008 17:26:16 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KZXxV-0003J4-QM for grub-devel@gnu.org; Sat, 30 Aug 2008 17:26:15 -0400 Received: from [199.232.76.173] (port=45976 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KZXxV-0003J1-KG for grub-devel@gnu.org; Sat, 30 Aug 2008 17:26:13 -0400 Received: from ey-out-1920.google.com ([74.125.78.148]:58776) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1KZXxV-0006wU-Ff for grub-devel@gnu.org; Sat, 30 Aug 2008 17:26:13 -0400 Received: by ey-out-1920.google.com with SMTP id 4so478628eyg.24 for ; Sat, 30 Aug 2008 14:26:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:subject:from:to:in-reply-to :references:content-type:date:message-id:mime-version:x-mailer; bh=XsQoPydfd9ujA3GLHRRTXq1s97pU/And3f+RlBhlXR8=; b=SnnumSn+4q7VvX7svdyvwoIE47E3qoWIiY/MKNPtBbWNOBsWMF5VpQyZ0kgQ5AvDHw IHWQWlMT27F8GxQjUXf2OJJXPuHm2twHBwpOM80miqLYHmkf7Y+D7q5kYrNT54Qfj/b5 BFQajWcZGKhA79wVG6nrFAfR3fwNi5VVQJQRQ= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=subject:from:to:in-reply-to:references:content-type:date:message-id :mime-version:x-mailer; b=oEOf9x5G5FHKEsTThsoP1pUrIFgvyklQizH1/L5yef2B+tXRARFpRfZVvlzk8RjJta Z2zmOEjJuNhgYZ6glc9JspVeM7ssgNfmBQJV1JXzNGt9TlSYI/bwaqwNTAF7UsQMKTjV pNKFCznyjg+bHFlKDXYvM7etSxCjPvgG1r8Jk= Received: by 10.210.105.19 with SMTP id d19mr3804090ebc.135.1220131571400; Sat, 30 Aug 2008 14:26:11 -0700 (PDT) Received: from ?192.168.1.100? ( [213.37.137.93]) by mx.google.com with ESMTPS id 7sm4576733eyb.1.2008.08.30.14.26.08 (version=SSLv3 cipher=RC4-MD5); Sat, 30 Aug 2008 14:26:10 -0700 (PDT) From: Javier =?ISO-8859-1?Q?Mart=EDn?= To: The development of GRUB 2 In-Reply-To: <20080830111753.GA16775@thorin> References: <20080704000829.GE4074@thorin> <1215135163.26019.44.camel@localhost> <20080704142125.GC2663@thorin> <1215182702.26019.130.camel@localhost> <20080704185723.GB32625@thorin> <1215204095.26019.142.camel@localhost> <20080705120757.GA1647@thorin> <1215282973.26019.183.camel@localhost> <20080719142707.GA23778@thorin> <20080830111753.GA16775@thorin> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-rui8x1/3bpf14+x9EwnC" Date: Sat, 30 Aug 2008 23:28:25 +0200 Message-Id: <1220131705.2622.19.camel@localhost> Mime-Version: 1.0 X-Mailer: Evolution 2.22.3.1 X-detected-kernel: by monty-python.gnu.org: Linux 2.6 (newer, 2) Subject: Re: grub-probe detects ext4 wronly as ext2 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: The development of GRUB 2 List-Id: The development of GRUB 2 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 30 Aug 2008 21:26:17 -0000 --=-rui8x1/3bpf14+x9EwnC Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Hello there! It's nice to be in town again, though post-vacation syndrome is hitting me full... Resuming the thread, El s=C3=A1b, 30-08-2008 a las 13:17 +0200, Robert Millan escribi=C3=B3: > On Mon, Aug 11, 2008 at 04:14:00PM +0200, Javier Mart=C3=ADn wrote: > > >> > This overrides the grub_errno and grub_errmsg provided by grub_dis= k_read and > > >> > replaces them with values that hide the true problem. If there wa= s a disk > > >> > read error, we really want to know about it from the lower layer. > > >> Well, the old version did just the same (even worse, because the mes= sage > > >> was generic). What would be the correct path of action here? I mean,= how > > >> can we propagate the error messages? > > > > > > It shouldn't call grub_error(). > >=20 > > So in the cases the fail is caused by an underlying error (like I/O) > > the code should just return failure and leave the old error in errno > > and errmsg? >=20 > Yes. Ok, so that's already done in the previous patch. >=20 > > static struct grub_ext2_data * > > grub_ext2_mount (grub_disk_t disk) > > { > > struct grub_ext2_data *data; > > + const char *local_error =3D 0; >=20 > Please use NULL for pointers. I don't object at all, but 0 is used throughout the GRUB code to represent null pointers (see the args struct in any command source file), so I don't understand the criterion being applied here. >=20 > > - if (grub_errno) > > + if (grub_errno !=3D GRUB_ERR_NONE) >=20 > Why? 1st, because the condition being checked is explicit and thus clearer. These int->bool C conventions are, even though enshrined by ANSI and thus pretty standard and reliable, irky at best and due only to the lack of a proper boolean type. As I think I stated before, the only cases I use such "cast" is when checking for null pointers and when using variables that are explicitly boolean in nature. 2nd, because the new code does not assume that GRUB_ERR_NONE is defined to zero. Even though this definition will most likely never change, we might one day decide that -42 is a better value for success. 3rd, because in addition to all that, with any sensible compiler, the additional binary size cost is nothing at all. >=20 > > - goto fail; > > + EXT2_DRIVER_MOUNT_FAIL(0); >=20 > I find very little use in this. I assume it's supposed to simplify thing= s, > but in fact it adds an extra level of indirection for someone who's readi= ng > the code. It provides no runtime improvement, and it's inconsistent with= what > we do elsewhere. It is not meant to provide any runtime improvement, it's just for consistency: since local_error is already zero, a "goto fail" would suffice but I think this uniformity adds readability, not hinders it: the referenced macro is at the very top of the function, not on a included header, so the reference is not very time-consuming; and it's not very complex, so it only needs to be read once. Besides, most compilers should just optimize the extra assignment away given that the value of local_error is ct-known for the code paths leading to that statement and it is not a "volatile" variable. >=20 > > + /* Only call grub_error if the fail was _not_ caused by underlying e= rrors. */ >=20 > No need to document this. It's the same deal in a huge amount of routine= s > throurough the GRUB source. OK, removed the comment. Maybe a similar comment will be fine over the macro, though. That was all, folks! Given that I (think I) addressed your two main objections, I won't send a new patch right now. I will continue to read the list this month, but my availability is likely to vary wildly, as I will be trapped by the ensnaring bureaucracy of the Spanish universities, scholarships, etc. -Habbit --=-rui8x1/3bpf14+x9EwnC Content-Type: application/pgp-signature; name=signature.asc Content-Description: Esta parte del mensaje =?ISO-8859-1?Q?est=E1?= firmada digitalmente -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iQIVAwUASLm7eaSl+Fbdeo72AQJgaA//Wz2oTlaLx78nrkILQZNCNf1d4U6aItnA qNUxdzYtJVVWNPV+8iXRKzIbMXWtGxKU6vWQk7XfoI/fJu68argVe7Kevdv7nwUo OHHFNlVhfjX+HqbBKtQyOdmPObJnTaCfpYBR+P3hBNEiTyZCpcuBS8G3X3XDaSCk fKaUVrA3UTXuGPke0ChxcoDSEj2mehB0T4ANrNXgZkTxkUdDTKLrKpoOgzxV8scr qqtI3Bp9jmOgiOsfx2EBlAIWRWjIV7Xzhx79i61t0hHDXhzbL4UbwPP//RxQlRyy 2nNFtSh3xh/pdoE4PMnFuPb/GfhmivVQjBHklGuIcT1DniCP0bHGt1haLqy9n8cR RRt8isaNmqYOVccS/xNurStMgD+9NyCXuOdzXVq9159tgACNGaGTR5X9ALm8LGFl Jukti2lyHF5OgmSsCcg2Ot4t8FMfvaMqmGZ8e53aHGloLsYlo2Ya4WvjHBHUX8Bu loxtTFhlxpCL696lposX/RH2RkabdNjGSjy5/Gg7fnFLU5m1HXzg+f+57PsHHP06 Rr+IGTbXiYBqQA0YpQOMa0chOowXNZm3vcj5PEgzFOAPnMfF70Hf4fwkoGtDuyqO lPxafuc8Oa5OTeHahYGxY1jnBLSAutk5XD+8c8bj+oJjztn6EQ9nlA/+B8dfDXs9 uCvs5nt5PqU= =HsCo -----END PGP SIGNATURE----- --=-rui8x1/3bpf14+x9EwnC--