Improve error handling for KERN_PROC_{CWD,FILEDESC}.

[markj]

Summary

We were returning 0 in places where the output was being truncated. I
couldn't find any consumers of KERN_PROC_CWD, but kinfo_getfile(3)
pretty clearly shouldn't be returning a truncated file array.

This change is incomplete, as most callers of export_vnode_to_sb() do
not check the return value and thus don't detect truncation. It's
sufficient to fix KERN_PROC_CWD though.

Test Plan

Will test tmux, as suggested by mjg@.

[mjg]

I have no opinion one way or the other.

tmux uses the feature and should be verified.

[cem]

I don't see how this affects tmux in any way, unless the error code breaks the previously working-but-truncated path(s). If that's the case, I think those should be changed to swallow ENOMEM. But then, what's the point of propagating the error up?

sys/kern/kern_descrip.c

4297–4300

Is the ordering of sbuf_bcat() -> sbuf_error() well-defined as used here? Or should this be an if()/else?

[markj]

In D15607#329598, @cem wrote:

I don't see how this affects tmux in any way, unless the error code breaks the previously working-but-truncated path(s). If that's the case, I think those should be changed to swallow ENOMEM. But then, what's the point of propagating the error up?

Looking at the tmux code, I don't believe this change will have an effect. It would just be a sanity test.

sys/kern/kern_descrip.c

4297–4300

The "?" operator is a sequence point, i.e., the first expression must be evaluated before the ones following "?".

[cem]

sys/kern/kern_descrip.c

4297–4300

I think I'd still prefer if/else for clarity. It doesn't fit in a single line anymore anyways.

[markj]

• Use an if statement.

[markj]

sys/kern/kern_descrip.c

4297–4300

Ok. It didn't fit before either (> 80 cols).

[cem]

Looks fine to me, with the caveat that I haven't looked at what might break with an error return. I'm most interested in the impact on core dumps.

sys/kern/kern_descrip.c

4297–4300

Heh, oops :-)

[jhb]

I do think it was intentional to truncate once the output buffer was exhausted rather than failing with ENOMEM. Its debatable if that is a useful thing or not. Most sysctls return ENOMEM instead so that the request can be retried with a larger output buffer so it is probably better to do what this does.

[cem]

In D15607#330347, @jhb wrote:

I do think it was intentional to truncate once the output buffer was exhausted rather than failing with ENOMEM. Its debatable if that is a useful thing or not. Most sysctls return ENOMEM instead so that the request can be retried with a larger output buffer so it is probably better to do what this does.

That makes sense in sysctl context. I'm not sure it makes sense in core dump context. Maybe it does, but it would probably require additional changes there.

[markj]

In D15607#330348, @cem wrote:

In D15607#330347, @jhb wrote:

I do think it was intentional to truncate once the output buffer was exhausted rather than failing with ENOMEM. Its debatable if that is a useful thing or not. Most sysctls return ENOMEM instead so that the request can be retried with a larger output buffer so it is probably better to do what this does.

That makes sense in sysctl context. I'm not sure it makes sense in core dump context. Maybe it does, but it would probably require additional changes there.

Exactly, and the error should be propagated up so that the right layer (e.g., sysctl vs. core dump) can decide whether to throw it away or not. As it happens, the core dump code already ignores these errors, so this change is for the benefit of sysctl interfaces.

[cem]

In D15607#330352, @markj wrote:

Exactly, and the error should be propagated up so that the right layer (e.g., sysctl vs. core dump) can decide whether to throw it away or not. As it happens, the core dump code already ignores these errors, so this change is for the benefit of sysctl interfaces.

Ah, perfect.

[allanjude]

This came up again via: https://blog.guillaume-gomez.fr/articles/2021-12-14+sysinfo%3A+version+0.22+and+FreeBSD+support

Is it good to commit?

[markj]

In D15607#756096, @allanjude wrote:

This came up again via: https://blog.guillaume-gomez.fr/articles/2021-12-14+sysinfo%3A+version+0.22+and+FreeBSD+support

Is it good to commit?

I think so. I'm doing some more testing and will commit it later today (with error checking added to export_vnode_to_sb() callers).