svn commit: r344238 - head/stand/common

[Rodney W. Grimes]

> On Mon, Feb 18, 2019 at 7:31 AM Ian Lepore <ian at 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 at FreeBSD.org> wrote:
> > > >
> > > > Author: ian
> > > > Date: Sun Feb 17 23:32:09 2019
> > > > New Revision: 344238
> > > > URL: https://svnweb.freebsd.org/changeset/base/344238
> > > >
> > > > Log:
> > > >  Restore loader(8)'s ability for lsdev to show partitions within a bsd
> > slice.
> > > >
> > > >  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.
> > > >
> > > >  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.
> > > >
> > > >  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.
> > >
> > >
> > > 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).
> > >
> > > Of course I can see the appeal for something like ?boot disk0:? 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:
> > >
> > > But anyhow, it would be good to understand the actual reasoning
> > > behind that decision.
> > >
> >
> > 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.
> >
> > 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 = -1,
> > PNUM_RAWSLICE = -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.
> >
> 
> 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.

Yes please.
Could this be some of the cause of issues with legacy mbr boots
that use to work, that now stop working?  I have observed this
on zfs in mbr on a machine here.  I did not have time to debug
it, it was faster to nuke and pave the machine that was critical
to operations, the error came as part of a failed freenas upgrade
to the 11.2 based version.

-- 
Rod Grimes                                                 rgrimes at freebsd.org