On Oct 23, 2013, at 6:18 PM, NeilBrown <neilb@xxxxxxx> wrote: > On Wed, 23 Oct 2013 12:36:23 -0500 Tony Asleson <tasleson@xxxxxxxxxx> wrote: > >> On 10/22/2013 08:44 PM, NeilBrown wrote: >>> On Tue, 22 Oct 2013 10:23:14 -0500 Tony Asleson <tasleson@xxxxxxxxxx> wrote: >>>> The reason I chose to return values was to make sure requested operation >>>> actually completed requested operation. Unexporting a non-existent >>>> export is not considered an error and returns no indication you did >>>> absolutely nothing. >>> >>> Hi, >>> thanks makes sense - I had missed that (even though you explained it in the >>> patch description :-( ) >>> >>> With your patch, if asked to unexport something that wasn't exported it >>> would not report any error, but would exit with an error status. Is that >>> correct? I think I would rather have a message printed if there is an error. >> >> Correct, I only made changes for the exit status. I was trying to make >> changes that would be mostly invisible to end users. I have no concerns >> adding a printed error output too, but others may. >> >> Changing the behavior of any command line tool is potentially >> problematic when scripted. >> >>> So would something like this (on top of my patch) address you need, or was >>> there something else I missed? >> >> Yes, this should work for the unexport fs case. >> >> However, the reason my patch was a little more invasive was to ensure >> that both the export and unexport paths were covered. >> >> For example, if the strdup call fails in function client_init, we fail >> the operation and return exit value of 0. Unlikely, but just the first >> example I stumbled across. > > I think it is a lot closer to "impossible" than just "unlikely". > malloc doesn't fail in this sort of context, the OOM killer kills something > off instead. > My personal preference is to replace all malloc/calloc/strdup calls with > the xmalloc, xstrdup etc calls in support/nfs/xcommon.c. > If you are worried about malloc failing, I'd much prefer to see a patch which > changes nfs-utils to use those uniformly. > > There might be a question over the best behaviour for daemons like mountd > and gssd. However as we move towards having systemd manage those, they will > be restarted if they ever exit, and they are mostly stateless so that is > quite safe. > > Does anyone else have thoughts on this? Yes. My thought is "xmalloc is an abomination." :-) We really do not want any of these tools exiting left if there's a memory allocation failure. For a user, that's no better than a segfault. What's more, if a utility like exportfs isn't very carefully coded, a sideways exit can leave on-disk files in an inconsistent state. A rule of thumb is never hide control flow (like exiting) inside macros or libraries. -- Chuck Lever chuck[dot]lever[at]oracle[dot]com -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
