nsexec: Don't ignore write failures

[rata]

If write fails for some reason or manages to write less than what we
requested (e.g. disk is full), report an error.

This silence the warning when compiling on github CI, as now the
write(2) return code is not ignored.

While we could do:
ret = write(.., ret)
if (ret < 0)

as other parts of the code does, there was recently a race condition
fixed in "libct/nsenter: fix logging race in nsexec" (2bab4a5) and
seems fragile to just test for -1. Let's check that we wrote exactly
what we expect, otherwise it won't be valid json written.

For the same reason, the race condition fixed, we don't retry if we
write partial data, as that uses two different write(2) calls which
might hit the same race. Therefore, if we failed to write what we
expect, we just let the caller know.

Note that if the write fails, it is possible for the (partial) json
written to not be valid.

Signed-off-by: Rodrigo Campos rodrigo@kinvolk.io

[rata]

Added a second commit to treat C warnings as errors, so the CI fails and is not so easy to miss these kind of errors in the future. Ready for review now! :)

I've tested that indeed the warning was treated as an error in the CI if I remove the "libct/nsenter: Don't ignore write failures" commit and keep "libct/nsenter: Treat CGO warnings as errors". For the CI to use the -Werror flag, it was not enough to make the change in the _gccgo.go file. I needed to do it in nsenter.go file too.

[tianon]

Given that what constitutes a "warning" varies from version to version of the compiler, shouldn't this be somehow added only in the CI instead? Otherwise I imagine a lot of downstream packagers might end up having to patch it out in the future. 😬

[kolyshkin]

Totally agree with @tianon -- it is discouraged to have -Werror in non-development builds, for the reason explained above.

It seems we can have CGO_CFLAGS=-Werror defined via environment in CI (and CI only). Maybe also add -Wextra (to nsenter.go, not via env, as otherwise runtime/cgo will fail to build, as it has -Werror -- and that's a nice demonstration of why -Werror can be bad).

[kolyshkin]

No, we can't have -Wextra:

# github.com/opencontainers/runc/libcontainer/nsenter
cloned_binary.c: In function ‘clone_binary’:
cloned_binary.c:511:21: warning: comparison of integer expressions of different signedness: ‘size_t’ {aka ‘long unsigned int’} and ‘__off_t’ {aka ‘long int’} [-Wsign-compare]
  511 |         while (sent < statbuf.st_size) {
      |                     ^
cloned_binary.c:522:18: warning: comparison of integer expressions of different signedness: ‘size_t’ {aka ‘long unsigned int’} and ‘__off_t’ {aka ‘long int’} [-Wsign-compare]
  522 |         if (sent != statbuf.st_size)
      |                  ^~
# github.com/opencontainers/runc/libcontainer/nsenter
nsexec.c: In function ‘write_file’:
nsexec.c:202:17: warning: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Wsign-compare]
  202 |         if (len != data_len) {
      |                 ^~
nsexec.c: In function ‘try_mapping_tool’:
nsexec.c:246:73: warning: unused parameter ‘map_len’ [-Wunused-parameter]
  246 | static int try_mapping_tool(const char *app, int pid, char *map, size_t map_len)
      |                                                                  ~~~~~~~^~~~~~~
# github.com/opencontainers/runc/libcontainer/nsenter
_cgo_main.c: In function ‘crosscall2’:
_cgo_main.c:2:23: warning: unused parameter ‘fn’ [-Wunused-parameter]
    2 | void crosscall2(void(*fn)(void*), void *a, int c, __SIZE_TYPE__ ctxt) { }
      |                 ~~~~~~^~~~~~~~~~
_cgo_main.c:2:41: warning: unused parameter ‘a’ [-Wunused-parameter]
    2 | void crosscall2(void(*fn)(void*), void *a, int c, __SIZE_TYPE__ ctxt) { }
      |                                   ~~~~~~^
_cgo_main.c:2:48: warning: unused parameter ‘c’ [-Wunused-parameter]
    2 | void crosscall2(void(*fn)(void*), void *a, int c, __SIZE_TYPE__ ctxt) { }
      |                                            ~~~~^
_cgo_main.c:2:65: warning: unused parameter ‘ctxt’ [-Wunused-parameter]
    2 | void crosscall2(void(*fn)(void*), void *a, int c, __SIZE_TYPE__ ctxt) { }
      |                                                                 ^
_cgo_main.c: In function ‘_cgo_release_context’:
_cgo_main.c:4:41: warning: unused parameter ‘ctxt’ [-Wunused-parameter]
    4 | void _cgo_release_context(__SIZE_TYPE__ ctxt) { }
      |                                         ^
_cgo_main.c: In function ‘_cgo_allocate’:
_cgo_main.c:6:26: warning: unused parameter ‘a’ [-Wunused-parameter]
    6 | void _cgo_allocate(void *a, int c) { }
      |                    ~~~~~~^
_cgo_main.c:6:33: warning: unused parameter ‘c’ [-Wunused-parameter]
    6 | void _cgo_allocate(void *a, int c) { }
      |                             ~~~~^
_cgo_main.c: In function ‘_cgo_panic’:
_cgo_main.c:7:23: warning: unused parameter ‘a’ [-Wunused-parameter]
    7 | void _cgo_panic(void *a, int c) { }
      |                 ~~~~~~^
_cgo_main.c:7:30: warning: unused parameter ‘c’ [-Wunused-parameter]
    7 | void _cgo_panic(void *a, int c) { }
      |                          ~~~~^

[rata]

@tianon @kolyshkin oh, right. Excellent point!

[kolyshkin]

The alternative to this one is #3154.

[kolyshkin]

This variable is unused below, iow this assignment is not needed.

[kolyshkin]

If write fails for some reason or manages to write less than what we
requested (e.g. disk is full), report an error.

We are not writing to disk here, we're writing to the pipe.

Therefore, if we failed to write what we
expect, we just let the caller know.

This function does not return anything, so this statement
is not accurate.

I was thinking about how we can handle a partial write,
and decided that we don't. With that in mind, seems like
the best solution is #3168; PTAL.

[rata]

@kolyshkin oh, thanks!

I was turning this PR into just treat warnings as errors on the CI. Wrote the code (it is quite simple), but then it can be quite painful when we upgrade the CI (like change the ubuntu version or the like, might reveal several new warnings). So, unsure if this has value or will be more pain than benefits.

If no one thinks we should do that change, I'll close this in a few days :)

[kolyshkin]

I was turning this PR into just treat warnings as errors on the CI. Wrote the code (it is quite simple), but then it can be quite painful when we upgrade the CI (like change the ubuntu version or the like, might reveal several new warnings). So, unsure if this has value or will be more pain than benefits.

I think adding -Werror to CI (and CI only) makes sense and adds value. If you want to work on it, please go ahead (I guess in a new PR, let's close this one).