Date: Mon, 18 Feb 2019 19:04:09 +0200 From: Toomas Soome <tsoome@me.com> To: Warner Losh <imp@bsdimp.com> Cc: Ian Lepore <ian@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r344238 - head/stand/common Message-ID: <7C165E48-DB8B-40F2-B9E9-2FB57C348252@me.com> In-Reply-To: <CANCZdfrs80%2B9_foTy_hDHcezgKwQ8RVJyc4GEJO9AiEpTAirEg@mail.gmail.com> References: <201902172332.x1HNW9ut059440@repo.freebsd.org> <2EAE5156-2C63-47FC-977F-0D9676CABF7F@me.com> <4283f17c1841561a41a03a9203ab295564aa0ba5.camel@freebsd.org> <CANCZdfrs80%2B9_foTy_hDHcezgKwQ8RVJyc4GEJO9AiEpTAirEg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
> On 18 Feb 2019, at 18:25, Warner Losh <imp@bsdimp.com> wrote: >=20 >=20 >=20 > On Mon, Feb 18, 2019 at 7:31 AM Ian Lepore <ian@freebsd.org = <mailto:ian@freebsd.org>> wrote: > On Mon, 2019-02-18 at 15:09 +0200, Toomas Soome wrote: > > > On 18 Feb 2019, at 01:32, Ian Lepore <ian@FreeBSD.org> wrote: > > >=20 > > > Author: ian > > > Date: Sun Feb 17 23:32:09 2019 > > > New Revision: 344238 > > > URL: https://svnweb.freebsd.org/changeset/base/344238 = <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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?7C165E48-DB8B-40F2-B9E9-2FB57C348252>