libstand: verify value provided by nfs.read_size
ClosedPublic

Authored by tsoome on Nov 10 2016, 1:49 PM.

Details

Summary

Implement simple value check and feedback.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
tsoome retitled this revision from to libstand: verify value provided by nfs.read_size.Nov 10 2016, 1:49 PM
tsoome updated this object.
tsoome edited the test plan for this revision. (Show Details)
tsoome added reviewers: allanjude, imp.
tsoome updated this revision to Diff 22126.Nov 10 2016, 1:53 PM

Should be *env == '\0' ...

julian accepted this revision.Nov 10 2016, 2:32 PM
julian added a reviewer: julian.
julian added a subscriber: julian.

looks reasonable, but am unsure of all the possible cases for *end given garbage input

This revision has a positive review.Nov 10 2016, 2:32 PM
tsoome updated this revision to Diff 22151.Nov 11 2016, 6:32 PM

Add notification about resetting the value for nfs_read_size.

This revision now requires review to proceed.Nov 11 2016, 6:32 PM
tsoome updated this revision to Diff 23192.Dec 22 2016, 9:43 AM

Factored out the set_nfs_read_size(), as the r305588 did miss the fact
we have not one but two versions of nfs_getrootfh() and we need to set
the nfs_read_size in both.

Since we can ignore the user provided value, reset the environment
with actually used value so we will not get the repeated messages about
bad value.

rpokala requested changes to this revision.Wed, Mar 15, 9:04 PM
rpokala added inline comments.
lib/libstand/nfs.c
228 ↗(On Diff #23192)

nfs_read_size is (int); while strtol() returns (long); cast accordingly.

229 ↗(On Diff #23192)

Is it legal to check for non-zero errno? I thought errno in general is only set to a non-zero value of failure, but not set to zero on success. So, a previous failure would set errno to non-zero, and it would stay non-zero even if the getenv() succeeded.

This revision now requires changes to proceed.Wed, Mar 15, 9:04 PM
tsoome updated this revision to Diff 26289.Wed, Mar 15, 9:16 PM
tsoome marked an inline comment as done.

Typo in errno name and casting to int.

tsoome added inline comments.Wed, Mar 15, 9:20 PM
lib/libstand/nfs.c
228 ↗(On Diff #23192)

well, the compiler should take care, but I do not mind anyhow:)

229 ↗(On Diff #23192)

I set it 0 at line 227 (modulo typo;), so w have it set 0 just before the call to strtol.

rpokala added inline comments.Wed, Mar 15, 9:30 PM
lib/libstand/nfs.c
229 ↗(On Diff #23192)

Right, but I'm not sure that's actually legal. I know in some contexts errno is actually a function-like macro or something wacky, so for example different threads don't stomp on it. Granted, that might not be relevant in the context of the bootloader. :-)

In any case, the rule I learned is "never set errno to zero, only to non-zero on failure".

tsoome added inline comments.Wed, Mar 15, 9:41 PM
lib/libstand/nfs.c
229 ↗(On Diff #23192)

Well yes, standalone boot is a special environment and as here we run as one big single threaded app, there is nothing to reset it for us and since libstand does not reset it either, we can have errno set anywhere else and not in strtol() so we can end up having errno == ERANGE from anywhere but strtol() itself:)

rpokala accepted this revision.Wed, Mar 15, 9:44 PM

Fair enough. :-)

This revision has a positive review.Wed, Mar 15, 9:44 PM
allanjude accepted this revision.Mon, Mar 20, 6:37 PM

Approved by: allanjude

This revision was automatically updated to reflect the committed changes.