From owner-svn-src-head@freebsd.org Mon Feb 18 16:25:14 2019 Return-Path: Delivered-To: svn-src-head@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 6752A14E15F0 for ; Mon, 18 Feb 2019 16:25:14 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-qt1-x82d.google.com (mail-qt1-x82d.google.com [IPv6:2607:f8b0:4864:20::82d]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id F01CF6F5ED for ; Mon, 18 Feb 2019 16:25:13 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-qt1-x82d.google.com with SMTP id z39so19859119qtz.0 for ; Mon, 18 Feb 2019 08:25:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=QeCGp8iAmq194NJteXxBukowUS86SPt0DnFmhPckR/o=; b=eUu+k3B3t6B14iZXYQIito4XMPE1wbFCtmGQm9wOr9pndQDtnX1j+lexB2EasTNhtc DJT+5HDJa05Wd2ye+AXw2dLd0IEVRNEwrx0bQZuMYnBxrEFgDNAuY6re3+eamrJc3nTy C8TwhigJoQDhOqCiDNyJ2CIPl54yzyCYroTri38jilcdmZ80Nuk9kjcw2q1vQbHxUIjy ihU7C43PSYTW+q08S76l00ZlrfAzZeqN/dyNgHSLgPt08oaKL9BpmXv7z0aXmlOES8rm u996x2SURy9mnrPyObxR5YDwZf+s8my0TLSbDfzpOzI73QDRXLEseANJngm9+PCsPbXK IHPQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=QeCGp8iAmq194NJteXxBukowUS86SPt0DnFmhPckR/o=; b=L2YsTLoGxoMAX+rzDunP7KxZZVtYjhd3ueuKGuYCtC41/SOUw3ARa7LHDTYuabrFZ1 o4o4XLh78vRuQEIl8aygE4ZRmIR/sr/g7mp6qhJrx69V8pLn7cPD5n0tJTfDYzDBj+nS mYR8Nf3atPk25Vw32gfUrA3qzOMEvQG2I+N/PwpF0jijDqW6P4ThuU7gv72gS0AbtdF+ OrInx1f57hMqdWbNPDQfI1fnmnqLPltfEMBGrJsWakSDjGxpDp4klnLZDzXyRuPBzOTD QGpju8kb2b1im5FNDSQTf3Hdhy+Bbkic1CAZvY0rYzkLPN9z6Yom0v90Gw/FSvYP7AJ1 bnpg== X-Gm-Message-State: AHQUAuZbsbsRAvql7IfdWN6l7CDl5r07MScfeQD0530axfRf13rFRye6 0IQ2UXVUA9SQDgZ11iIQ8EwHiaFd8MyGPNIMgEu5beww X-Google-Smtp-Source: AHgI3IbNOcaseM4hErMVKJ3jW3n665/t+rVKBK5zFJnBVCrmAg4FjaSO3F8P2BKA+xuuIRP8qtpwQe+WQvrZioa5T0w= X-Received: by 2002:a0c:b068:: with SMTP id l37mr18318816qvc.21.1550507113451; Mon, 18 Feb 2019 08:25:13 -0800 (PST) MIME-Version: 1.0 References: <201902172332.x1HNW9ut059440@repo.freebsd.org> <2EAE5156-2C63-47FC-977F-0D9676CABF7F@me.com> <4283f17c1841561a41a03a9203ab295564aa0ba5.camel@freebsd.org> In-Reply-To: <4283f17c1841561a41a03a9203ab295564aa0ba5.camel@freebsd.org> From: Warner Losh Date: Mon, 18 Feb 2019 09:25:02 -0700 Message-ID: Subject: Re: svn commit: r344238 - head/stand/common To: Ian Lepore Cc: Toomas Soome , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org X-Rspamd-Queue-Id: F01CF6F5ED X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.93 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_SHORT(-0.93)[-0.927,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-head@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 Feb 2019 16:25:14 -0000 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: > > > > > > 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 bs= d > 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 sta= rt > > > 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 rever= t > 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 =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: > > > > 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 =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. > 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. Warner