Handle kd == NULL gracefully with kvm_geterr(3)

[ngie]

Summary

Handle kd == NULL gracefully with kvm_geterr

Don't just work by accident with kvm_geterr(3), but instead return a
NUL string.

Document the new explicit return behavior for kvm_geterr(3), as well
as the previous implicit behavior, i.e., the buffer attached to
returned via kvm_geterr(3) would be empty.

MFC after: 1 week
Sponsored by: Dell EMC Isilon

[ngie]

lib/libkvm/kvm.c

293 ↗

(On Diff #26299)

Looking at the rest of the surrounding code, I should restore the cast and remove all of the unnecessary void* casts in another commit.

[ngie]

Remove unnecessary stylistic diff in kvm_close(3)

lib/libkvm/kvm_open.3

236–238 ↗

(On Diff #26301)

This sounds weird now that I read it out loud. Let me rewrite it.

[ngie]

• Fix error description for kvm_close relating to close(2).
• Reference close(2) in SEE ALSO.

[ngie]

Describe return value behavior, post this change.

[ngie]

Add some more kvm_geterr(3) tests

[ngie]

In D10022#207066, @ngie wrote:

Add some more kvm_geterr(3) tests

Please ignore this comment. There's no change in this diff vs the last one.

[ngie]

Would anyone on the CC list have time to review this change?

[vangyzen]

lib/libkvm/kvm.c

74 ↗

(On Diff #26306)

In the code in head, kvm_geterr never returns NULL. This path should return an empty string to preserve that API. This is usually(?) called on an error path, so it's harder to reach in testing and more likely to be used incorrectly (i.e. without testing for NULL).

302 ↗

(On Diff #26306)

return (error)

lib/libkvm/kvm_geterr.3

58 ↗

(On Diff #26306)

"The kvm_geterr function..."

62 ↗

(On Diff #26306)

Just kd, not *kd.

65 ↗

(On Diff #26306)

This is not true: kd->errbuf is a char[] initialized by calloc(), so kvm_geterr will always return a string if kd != NULL.

66 ↗

(On Diff #26306)

Maybe just kd here, too.

lib/libkvm/kvm_open.3

247 ↗

(On Diff #26306)

*kd -> kd

[ngie]

Update per feedback from vangyzen

[ngie]

I forgot to include the kvm.c changes in the previous diff

[ngie]

Don't return strerror(EINVAL) if kd == NULL in kvm_geterr... this wasn't previously
discussed/requested (it was just something I was trying out)

[ngie]

lib/libkvm/kvm.c

74 ↗

(On Diff #26306)

I didn't initially understand the point in doing this, but given that errbuf isn't guaranteed to be allocated on the stack, "" is the proper way to deal with this issue.

302 ↗

(On Diff #26306)

Good eye. I'll commit this separately after I do some more testing.

[ngie]

lib/libkvm/kvm.c

302 ↗

(On Diff #26306)

Committed as rS315603.

[vangyzen]

lib/libkvm/kvm.c

74 ↗

(On Diff #26306)

The type of "" is const, so the compiler should be warning that the return type discards the const qualifier (unless warnings are dialed down in libkvm). A more proper fix is something like:

static char kd_is_null[1];

Even better than that, make the return type const. Obviously, that could snowball into a larger change, but it would be beneficial. For example, you might find some callers that free() the returned string, since that would emit a warning about freeing a const pointer.

[jhb]

I actually think it's fine to return 'strerror(EINVAL)' instead of an empty string. The caller probably doesn't expect any empty string. Also, I don't think it's but so bad to return NULL as passing NULL to kvm_geterr() is undefined behavior anyway. Arguably you deserve to crash at that point.

[ngie]

lib/libkvm/kvm.c

74 ↗

(On Diff #26306)

Good point. I disagree on making it static though -- it should be externalized via kvm.h and testable.

With the const change, I think it's admirable, but it shouldn't be done in this commit. AFAICT, changing the lib call signature is ok, if the reasoning makes sense.

[ngie]

In D10022#208128, @jhb wrote:

I actually think it's fine to return 'strerror(EINVAL)' instead of an empty string. The caller probably doesn't expect any empty string. Also, I don't think it's but so bad to return NULL as passing NULL to kvm_geterr() is undefined behavior anyway. Arguably you deserve to crash at that point.

Hmm.. I'll think about this a bit. In retrospect (even though the behavior changes seemed to be benign and the changes opportunistic), I think the changes should be split up because functionally they're different.

Please hold a sec. I'll file up another review for kvm_close(3) and reuse this for kvm_geterr(3), only.

[vangyzen]

lib/libkvm/kvm.c

74 ↗

(On Diff #26306)

I don't understand the reasoning behind making it external. It's just a way to return an empty string. Callers don't need to test whether the pointer points to this particular empty string.

I agree that const should be a separate commit.

[ngie]

Reduce diff to just kvm_geterr(3) to get to resolution with the new behavior

[ngie]

Apply recommendations from vangyzen

As suspected, bumping up WARNS with kvm_geterr will fail after this
commit, but no more than it would have previously.

I'm working on bumping WARNS up to 6 though (see D10071, etc)

[ngie]

lib/libkvm/kvm.c

74 ↗

(On Diff #26306)

Yup... const poisoning here doesn't work quite yet... will fix in future commits (I have to __DECONST the return value from kvm_read2 too in order to calculate the ssize_t value returned from the function.