Date: Thu, 6 Jun 2013 21:21:35 -0700 From: Craig Rodrigues <rodrigc@FreeBSD.org> To: Ian Lepore <ian@freebsd.org> Cc: Rick Macklem <rmacklem@uoguelph.ca>, Lars Eggert <lars@netapp.com>, freebsd-current <freebsd-current@freebsd.org> Subject: Re: mounting root from NFS via ROOTDEVNAME Message-ID: <CAG=rPVdnbKjtKHccRK3918oHZKWX06LyL1%2By3bH5oZDNqiD87g@mail.gmail.com> In-Reply-To: <1370537672.1086.30.camel@revolution.hippie.lan> References: <433498856.127008.1370217441419.JavaMail.root@erie.cs.uoguelph.ca> <1370537672.1086.30.camel@revolution.hippie.lan>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Jun 6, 2013 at 9:54 AM, Ian Lepore <ian@freebsd.org> wrote: > Well, I started out just testing Lars' patch (it works) but that > inspired me to pick up the work I toyed with months ago in this area, to > try to get BOOTP_NFSROOT to respect other root-path options such as > setting vfs.root.mountfrom in the environment and using the RB_DFLTROOT > boot option. The attached patch does those things, as follows: > Your patch is better than the existing code. Your patch makes the code more readable. In my earlier response on this thread, I threw out the idea of axing ROOTDEVNAME from the kernel config options. I had a feeling that there would be opposition to it, so I'm OK with it staying. What I don't like to see is a lot of code behind conditional preprocessor logic of #ifdef ROOTDEVNAME. This often leads to bitrot if ROOTDEVNAME is not part of the GENERIC kernel config. However, with your patch, the amount of code inside #ifdef ROOTDEVNAME blocks is very minimal, so I can live with that. With your patch, we can trigger all the behavior by setting vfs.mountfrom in loader.conf with an "nfs:" prefix, so that is good, in terms of testing the feature with GENERIC kernels. I would say, if Lars Eggert has time to provide feedback and is OK with your patch, then go with your patch and commit it. The only minor feedback I would give is that in this comment: + /* + * Choose a root filesystem. If a value is forced in the environment + * and it contains "nfs:", use it unconditionally. Otherwise, use + * ROOTDEVNAME if it contains "nfs:" and either the server didn't + * provide a name or the boot options say to force ROOTDEVNAME. + */ + If you could take the sentence starting with "Otherwise, use", and maybe split it up into two sentences, that would make the comment a little bit more readable. Thanks. -- Craig
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAG=rPVdnbKjtKHccRK3918oHZKWX06LyL1%2By3bH5oZDNqiD87g>