From owner-svn-src-all@freebsd.org Mon Feb 18 17:04:23 2019 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id C568B14E3E27 for ; Mon, 18 Feb 2019 17:04:22 +0000 (UTC) (envelope-from tsoome@me.com) Received: from pv50p00im-tydg10011801.me.com (pv50p00im-tydg10011801.me.com [17.58.6.52]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4C1197239A for ; Mon, 18 Feb 2019 17:04:22 +0000 (UTC) (envelope-from tsoome@me.com) Received: from nazgul.lan (148-52-235-80.sta.estpak.ee [80.235.52.148]) by pv50p00im-tydg10011801.me.com (Postfix) with ESMTPSA id 205726601B8; Mon, 18 Feb 2019 17:04:11 +0000 (UTC) From: Toomas Soome Message-Id: <7C165E48-DB8B-40F2-B9E9-2FB57C348252@me.com> Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: Re: svn commit: r344238 - head/stand/common Date: Mon, 18 Feb 2019 19:04:09 +0200 In-Reply-To: Cc: Ian Lepore , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org To: Warner Losh References: <201902172332.x1HNW9ut059440@repo.freebsd.org> <2EAE5156-2C63-47FC-977F-0D9676CABF7F@me.com> <4283f17c1841561a41a03a9203ab295564aa0ba5.camel@freebsd.org> X-Mailer: Apple Mail (2.3445.102.3) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:, , definitions=2019-02-18_13:, , signatures=0 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 malwarescore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 mlxscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1812120000 definitions=main-1902180127 X-Rspamd-Queue-Id: 4C1197239A X-Spamd-Bar: ------ X-Spamd-Result: default: False [-6.99 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_SHORT(-0.99)[-0.992,0]; REPLY(-4.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0] Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 Feb 2019 17:04:23 -0000 > On 18 Feb 2019, at 18:25, Warner Losh wrote: >=20 >=20 >=20 > On Mon, Feb 18, 2019 at 7:31 AM Ian Lepore > wrote: > On Mon, 2019-02-18 at 15:09 +0200, Toomas Soome wrote: > > > On 18 Feb 2019, at 01:32, Ian Lepore wrote: > > >=20 > > > Author: ian > > > Date: Sun Feb 17 23:32:09 2019 > > > New Revision: 344238 > > > URL: https://svnweb.freebsd.org/changeset/base/344238 = > > >=20 > > > Log: > > > Restore loader(8)'s ability for lsdev to show partitions within a = bsd slice. > > >=20 > > > I'm pretty sure this used to work at one time, perhaps long ago. = It has > > > been failing recently because if you call disk_open() with = dev->d_partition > > > set to -1 when d_slice refers to a bsd slice, it assumes you want = it to > > > open the first partition within that slice. When you then pass = that open > > > dev instance to ptable_open(), it tries to read the start of the = 'a' > > > partition and decides there is no recognizable partition type = there. > > >=20 > > > This restores the old functionality by resetting d_offset to the = start > > > of the raw slice after disk_open() returns. For good measure, = d_partition > > > is also set back to -1, although that doesn't currently affect = anything. > > >=20 > > > I would have preferred to make disk_open() avoid such rude = assumptions and > > > if you ask for partition -1 you get the raw slice. But the = commit history > > > shows that someone already did that once (r239058), and had to = revert it > > > (r239232), so I didn't even try to go down that road. > >=20 > >=20 > > What was the reason for the revert? I still do think the current > > disk_open() approach is not good because it does break the promise = to > > get MBR partition (see common/disk.h).=20 > >=20 > > Of course I can see the appeal for something like =E2=80=9Cboot = disk0:=E2=80=9D but > > case like that should be solved by iterating partition table, and = not > > by making API to do wrong thing - if I did ask to for disk0s0: I > > really would expect to get disk0s0: and not disk0s0a: > >=20 > > But anyhow, it would be good to understand the actual reasoning > > behind that decision. > >=20 >=20 > I have no idea. As is so often the case, the commit message for the > revert said what the commit did ("revert to historic behavior") = without > any hint of why the change was made. One has to assume that it broke > some working cases and people complained. >=20 > Part of the problem for the disk_open() "api" is that there is not = even > a comment block with some hints in it. I was thinking one potential > solution might instead of using "if (partition < 0)" we could define a > couple magical partition number values, PNUM_GETBEST =3D -1, > PNUM_RAWSLICE =3D -2, that sort of thing. But it would require = carefully > combing through the existing code looking at all calls to disk_open() > and all usage of disk_devdesc.d_partition. >=20 > I think that we should fix the disk_open() api. And then fix = everything that uses it to comply with the new rules. The current = hodge-podge that we have causes a number of issues for different = environments that are only mostly worked around. I've done some work in = this area to make things more regular, but it's still a dog's breakfast = of almost-stackable devices that we overload too many things on, = resulting in the mis-mash we have today. When the whole world was just = MBR + BSD label, we could get away with it. But now that we have GPT as = well as GELI support and ZFS pool devices on top of disk devices, the = arrangements aren't working as well as could be desired. >=20 > Warner I am looking on it too, but it has more traps than I was suspecting:) rgds, toomas