Date: Mon, 14 Oct 2013 01:42:28 -0500 (CDT) From: Nathan Whitehorn <nwhitehorn@freebsd.org> To: Devin Teske <dteske@FreeBSD.org> Cc: Nathan Whitehorn <nwhitehorn@anacreon.physics.wisc.edu>, "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "src-committers@freebsd.org" <src-committers@freebsd.org>, "Teske, Devin" <Devin.Teske@fisglobal.com> Subject: Re: svn commit: r256343 - in head/usr.sbin/bsdinstall: . scripts Message-ID: <alpine.BSF.2.00.1310140127010.44110@anacreon.physics.wisc.edu> In-Reply-To: <13CA24D6AB415D428143D44749F57D720FC60628@LTCFISWMSGMB21.FNFIS.com> References: <201310112041.r9BKfZeT002056@svn.freebsd.org> <5258F9B3.7030101@freebsd.org> <13CA24D6AB415D428143D44749F57D720FC5BB95@LTCFISWMSGMB21.FNFIS.com> <alpine.BSF.2.00.1310130232370.33798@anacreon.physics.wisc.edu> <13CA24D6AB415D428143D44749F57D720FC60628@LTCFISWMSGMB21.FNFIS.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Devin, In this thread, I have made the following requests: 1. I suggested that camcontrol is not the right place to get disk ident strings, since not all disks are CAM, and suggested an alternative. 2. I pointed out a possible bug related to MBR setup and suggested a solution. 3. I asked you to update the man page to reflect the new features, as well as update a few other scripts to allow them to use your ZFS code. 4. I requested a somewhat longer testing period and additional review for future significant installer changes like this in the run-up to a release and tried to explain my rationale. 5. I suggested that the "Submitted by" and "Reviewed by" should probably not be the same person. I also thanked you for the patch, which adds a sorely-missed feature to the installer. I do not believe that any of these minor requests were unreasonable. Certainly, I do not see that they merit the vitriol that follows here. Since it doesn't seem to help with anything, I won't send any more mail on this. The one thing I have done is to ask re@ to ensure that, following the usual conventions, all non-trivial patches to the installer proposed for merge to stable/10 be reviewed and have a reasonable testing period in HEAD or on mailing lists before being MFC'ed for the remainder of the 10.0 release process. -Nathan On Sun, 13 Oct 2013, Teske, Devin wrote: > > On Oct 13, 2013, at 12:41 AM, Nathan Whitehorn wrote: > >> >> >> On Sat, 12 Oct 2013, Teske, Devin wrote: >> >>> >>> On Oct 12, 2013, at 12:26 AM, Nathan Whitehorn wrote: >>> >>>> On 10/11/13 22:41, Devin Teske wrote: >>>>> Author: dteske >>>>> Date: Fri Oct 11 20:41:35 2013 >>>>> New Revision: 256343 >>>>> URL: https://urldefense.proofpoint.com/v1/url?u=http://svnweb.freebsd.org/changeset/base/256343&k=%2FbkpAUdJWZuiTILCq%2FFnQg%3D%3D%0A&r=Mrjs6vR4%2Faj2Ns9%2FssHJjg%3D%3D%0A&m=LDzuPpXPP4D5BzfISZjw%2BXitYn4aKVzfXzcrmMNFo2U%3D%0A&s=3d0963d9c497f7bad0918888032ca62844580612dc48ab3a8a6768fe640c365b >>>>> >>>>> Log: >>>>> Add zfsboot module as an option for automatic configuration. Default is >>>>> to run interactively but it can be scripted too (optinally completely >>>>> non-interactive). Currently supports GELI and all ZFS vdev types. Also >>>>> performs validation on selections/settings providing error messages if >>>>> necessary, explaining (in plain language) what the issue is. Currently >>>>> the auto partitioning of naked disks only supports GPT and MBR (VTOC8 >>>>> pending for sparc64), so is only available for i386/amd64 install. >>>>> >>>>> Submitted by: Allan Jude <freebsd@allanjude.com>, myself >>>>> Reviewed by: Allan Jude <freebsd@allanjude.com> >>>>> Approved by: re (glebius) >>>> >>>> Hi Devin -- >>>> >>>> As was discussed on the mailing list, this patch still has some issues >>>> that need to be resolved, >>> >>> Can you kindly provide links? I'm crawling through the mailing lists and >>> not finding anything for the October, (current, stable, sysinstall, ... ?? others?) >>> >>> Do I need to be looking back in September? I wouldn't think so, because that >>> bit wasn't even in our development tree until October 1st: >> >> This was discussion on freebsd-current from yesterday and the day before. >> > > Links or it didn't happen. > > HINT: I just (for the the third time on this topic) crawled the following: > http://lists.freebsd.org/pipermail/freebsd-current/2013-October/thread.html > > Please help me find what you're talking about. > > > > >>> https://urldefense.proofpoint.com/v1/url?u=http://druidbsd.cvs.sf.net/viewvc/druidbsd/bsdinstall_zfs/usr.sbin%253A%253Absdconfig%253A%253Ashare%253A%253Adevice.subr.patch?revision%3D1.1%26view%3Dmarkup&k=%2FbkpAUdJWZuiTILCq%2FFnQg%3D%3D%0A&r=LTzUWWrRnz2iN3PtHDubWRSAh9itVJ%2BMUcNBCQ4tyeo%3D%0A&m=aas9lyFMptmk4eQ3C74XXVkRHbiN19EOClxMMjoCuhE%3D%0A&s=9d21b812e49d921455e998bde7c538318c3bfcc8759c1d0b61eede553b203865 >>> >>> So there couldn't have been any discussion on it before then. So I'm just not >>> able to find the mailing lists where all the action is that they're discussing it. >>> Would be nice to find where the action is, so I could participate. >>> >>> >>>> for example the use of camcontrol >>>> unconditionally even when the disks may not be CAM >>> >>> Allan Adds: >>> 9.2 should have all disks listed in camcontrol, so it shouldn't be an issue >> >> No it shouldn't. Not all disks are interfaced to CAM. MFI comes to mind, nvme, VM block devices, SD cards. There are many other examples. > > OK... duly noted. Much thanks for dispelling that myth. > > > >> Just because you don't have them does not justify a phenomenological approach here. >> > > Need I remind you... we don't *list* the disks from camcontrol... we just use it as a perfunctory > value-add by stealing disk descriptions from it if/when it is describing a disk. > > I would hardly call that phenomenological (rather, more of a multi-pass cartesian ontological > approach). > > > > >>> And: >>> I think the only systems without cam based disks are old 8.x - we're only targeting 10 anyway. >> >> Not true at all. >> > > Thanks. > > > > >>> I tend to agree with those statements. >>> >>> >>>> and destruction of >>>> existing sub-partitioning for MBR disks. >>> >>> I think we both (Allan and I) actually responded directly to you on this one. >>> >>> We have code that handles that. It's in there. >> >> To me, yes, but I was wrong in my initial comment as pointed out by some others. In particular, you need to run gpart -F destroy recursively on the disk instead of just on the root node. There were several other issues and bugs mentioned in people's cursory review of the patch. > > Where? by whom? and when? > > We've already established that it wasn't in the mailing lists (or at least not in -current as you claim). > > > >> I never dreamed you would then just commit it. >> > > A great mentor of mine once said... > > "Practice makes Perfect? NO! Unattainable. [pause] Practice makes *improvement*." > > Perfection versus Improvement. > > I would be hard-pressed to accept any argument that a regression has occurred. > > > >> I really really don't want to have to subject installer changes to explicit approval requirements, > > Threats won't help your cause. Civility is key. > > >> but *please* request review of non-trivial changes before commits, especially to the disk partitioning code, > > Didn't touch your disk partitioning code. > > I have zero plans to. You can keep it (I'll just replace it in-whole when I have something better). > > > >> and especially again before insta-MFCing them to a stable branch right before a release. > > Gleb gave me insta-MFC approval. > > >> It is much, much better than having to do this after the fact as >> we are doing now. > > What we are doing doesn't need to be done (period). > -- > Devin > > _____________ > The information contained in this message is proprietary and/or confidential. If you are not the intended recipient, please: (i) delete the message and all copies; (ii) do not disclose, distribute or use the message in any manner; and (iii) notify the sender immediately. In addition, please be aware that any message addressed to our domain is subject to archiving and review by persons other than the intended recipient. Thank you. > >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?alpine.BSF.2.00.1310140127010.44110>