savecore: fix space calculation with respect to `minfree` in check_space(..)

[ngie]

Summary

savecore: fix space calculation with respect to minfree in check_space(..)

• Use strtoll(3) instead of atoi(3), because atoi(3) limits the representable data to INT_MAX. Check the values received from strtoll(3), trimming trailing whitespace off the end to maintain POLA.
• Store available number of kB in available so it can be more easily queried and compared to ensure that there are enough kB to store the dump image on disk.
• Print out the reserved space on disk, per minfree, so end-users can troubleshoot why check_space(..) is reporting that there isn't enough free space.

MFC after: 7 weeks
Tested with: positive/negative cases; make tinderbox
Sponsored by: Dell EMC Isilon

Test Plan

Tested with both positive and negative scenarios, as noted below:

Positive cases:

• No minfree file (0kB).
• minfree set to "1024" (no newline) (1MB).
• minfree set to "1024\n" (with newline) (1MB).
• minfree set to the total space on the filesystem -> fail, noting that there isn't enough space.

Negative cases:

• minfree empty (0kB assumed)
• minfree with "1024ab" (warning printed; 0kB assumed)
• minfree with "aba" (warning printed; 0kB assumed)

[cem]

Do you plan to write kyua/atf tests for the testing? It seems like something that could be useful.

You could do it without needing a live panic by compressing the contents of a partition with a small coredump (idle after single user boot?) saved to it, then decompressing that into an md device in your test setup. The tests would savecore from the md device, modulating the contents of minfree.

[cem]

sbin/savecore/savecore.c

284 ↗

(On Diff #27394)

I think off_t was fine for at least spacefree and totfree. (It's int64_t at the end of the day, anyway.) Probably also the right type for the new available variable.

290–291 ↗

(On Diff #27394)

It seems to me like this calculation was more correct before the change.

off_t is int64_t on all platforms. Further more, the multiplication won't overflow with any off_t-representable disk. Finally, dividing f_bsize by 1024 discards precision with odd (not a multiple of 1k) and small (<1k) block sizes.

301–304 ↗

(On Diff #27394)

Would strtonum() be simpler here?

315 ↗

(On Diff #27394)

I'm not sure this distinction makes any sense. If minfree is non-zero, the administrator-reserved space must also be kept available. But otherwise we totally ignore administrator-reserved space? Why not just use spacefree (aka f_bavail) for both limits?

321 ↗

(On Diff #27394)

I'd suggest using humanize_number(3), although I realize that's a bigger change. This is fine as-is for now.

Not sure what FreeBSD convention is, but "kB" here refers to multiples of 1024, while the SI unit kB refers to multiples of 1000. So some places would spell that "kiB." I don't know if the project has chosen a preference one way or the other for this kind of thing.

[ngie]

In D10379#215002, @cem wrote:

Do you plan to write kyua/atf tests for the testing? It seems like something that could be useful.

Yup. I need to write a "crash dump emulator" though.

You could do it without needing a live panic by compressing the contents of a partition with a small coredump (idle after single user boot?) saved to it, then decompressing that into an md device in your test setup. The tests would savecore from the md device, modulating the contents of minfree.

I could, but then we'd be checking in more binary junk to the tree :( (plus it wouldn't work for Isilon with the compress mini dumps functionality, right?)..

[cem]

In D10379#215008, @ngie wrote:

In D10379#215002, @cem wrote:

Do you plan to write kyua/atf tests for the testing? It seems like something that could be useful.

Yup. I need to write a "crash dump emulator" though.

My April fools' day idea for this year was to write a savecore frontend called restorecore to put the crash dumps back into the disk. That's one kind of crash dump emulator :-).

You could do it without needing a live panic by compressing the contents of a partition with a small coredump (idle after single user boot?) saved to it, then decompressing that into an md device in your test setup. The tests would savecore from the md device, modulating the contents of minfree.

I could, but then we'd be checking in more binary junk to the tree :(

Yeah, that's true. I suspect the size of a compressed idle crash dump would be pretty tiny, but have no evidence of that.

(plus it wouldn't work for Isilon with the compress mini dumps functionality, right?)..

It'd test the functionality of minfree either way. Some thought might be needed to also test Isilon's cores and savecore behavior, but I like to solve one problem at a time.

[ngie]

sbin/savecore/savecore.c

284 ↗

(On Diff #27394)

ACK. I was thinking 32-bit vs 64-bit on Linux I guess (off_t was 32-bit, then off64_t was introduced, then off64_t was phased out and off_t became 64-bit).

301–304 ↗

(On Diff #27394)

I forgot about strtonum. It wouldn't really be any better IMHO seeing as it only trims off leading whitespace and not trailing whitespace.

Also, + prefixing isn't documented in minfree, and I'm not sure that should be supported.

315 ↗

(On Diff #27394)

That's a phk@ question I don't know the answer to (minfree as it stands deals with user-available space, not administrator reserved space, per savecore(8)).

321 ↗

(On Diff #27394)

Right. I considered humanize_number(3), but I didn't want to make that change just here (there are a couple other spots where humanize_number(3) should probably be sprinkled around).

humanize_number(3) references Ki, which technically isn't an SI unit (the K should be k). A really quick grep suggests that "<n> kB" is the defacto way of representing "<n> * 1024 bytes".

[ngie]

Update based on some of cem's feedback:

• revert off_t -> int64_t type changes.
• make available off_t for consistency with spacefree and totspace.

[cem]

sbin/savecore/savecore.c

306–308 ↗

(On Diff #27395)

Maybe log the bogus contents of buf? Maybe not.

284 ↗

(On Diff #27394)

Yeah, Linux still needs -D_FILE_OFFSET_BITS=64 to get a 64-bit off_t, which is kind of nuts. Not a problem we have, fortunately.

301–304 ↗

(On Diff #27394)

ACK.

[rang_acm.org]

sbin/savecore/savecore.c

305 ↗

(On Diff #27395)

I think you should also check for a range error here (errno == ERANGE).

321 ↗

(On Diff #27394)

Ki isn’t an SI unit, but it’s an IEC prefix (see http://www.iec.ch/si/binary.htm).

So KiB = 1024 bytes. I only see it in a handful of places so far in the tree, but we can help move in that direction. If we’re not using humanize_number(3), and I agree that we don’t need to yet, then we can use the KiB unit. (For disks, I’ve been used to seeing KB in the past, but … standards.)

[cem]

sbin/savecore/savecore.c

321 ↗

(On Diff #27394)

Maybe, but I think kiB should match kB — rather than capitalizing the k. KB is definitely wrong.

[rang_acm.org]

sbin/savecore/savecore.c

303 ↗

(On Diff #27395)

To be pedantic, the argument to isspace() should be an unsigned char, not a char; or to be extremely pedantic, either EOF or a value representable as an unsigned char. This matters for compilers where the default char is signed and isspace() is implemented as a lookup table. In our _ctype.h, isspace() is written in such a way that it will always return false when passed a negative value, so it doesn’t matter, but isspace((unsigned char)*endp) would be more correct.

It looks like most FreeBSD code doesn’t worry about this, so I’m OK if you don’t want to change it.

[rang_acm.org]

sbin/savecore/savecore.c

321 ↗

(On Diff #27394)

Well, Ki is the standard now, for better or worse. (As best I can tell without paying the $188 for an official copy of the standard.) :-/ It’s also what humanize_number(3) uses, so we’d be moving in that direction.

[cem]

sbin/savecore/savecore.c

321 ↗

(On Diff #27394)

Ugh, that's awful. Just sticking with kB to represent 1024 bytes might be less bad.

[ngie]

sbin/savecore/savecore.c

303 ↗

(On Diff #27395)

Technically, it's isspace(int), not isspace(unsigned char), per FreeBSD, Linux, and OSX. I don't think changing the type from char to int is going to really buy us much to be honest, so I'm going to ignore this.

305 ↗

(On Diff #27395)

Thanks! I thought I might have been forgetting something!

306–308 ↗

(On Diff #27395)

Sure!

[ngie]

Update per some of arang's and cem's recommendations

[ngie]

sbin/savecore/savecore.c

321 ↗

(On Diff #27394)

Gosh darn it. Ok, I'll change this to KiB :(...

[ngie]

Use KiB for kibibyte (1024) as kB represents 1000, not 1024 like I originally thought.

[ngie]

Ping? Is there anything else you'd like for me to do to improve the change in the short-term?