Document error handling convention

[eriksjolund]

Document the error handling convention used in the crun source code. The documentation could for example be added to the file CONTRIBUTING.md or some new file.

Just to give an idea what I'm thinking about, here is a sketch:

• a function that takes an argument libcrun_error_t *err and returns a negative return value, creates an error in err. The function does not check if a previously created error is overwritten. To avoid a memory leak, callers of the function should thus pass in an err that has not already been created.

• a function that takes an argument libcrun_error_t *err and returns a non-negative return value, does not create an error in err.

By having such error handing convention, the last line in the following code would be unnecessary:

libcrun_error_t tmp_err = NULL;
ret = some_crun_function (&tmp_err);
if (ret < 0) {
   if (tmp_err != NULL) {

[giuseppe]

err must never be not-NULL and passed as an argument to a function. As soon as it has a value (because an error happened), it is returned to the caller. The code:

libcrun_error_t tmp_err = NULL;
ret = some_crun_function (&tmp_err);
if (ret < 0) {
   if (tmp_err != NULL) {

should never happen.

Or are you mentioning the cleanup of err when using a temporary tmp_err?

[eriksjolund]

The code:

libcrun_error_t tmp_err = NULL; ret = some_crun_function (&tmp_err); if (ret < 0) { if (tmp_err != NULL) {

should never happen.

agree

Or are you mentioning the cleanup of err when using a temporary tmp_err?

That could also be mentioned in such error handling documentation.

I got the idea of creating this issue when I saw the following code in https://github.com/containers/crun/pull/1859

      ret = do_mount (container, empty_dir_path, pathfd, rel_path, NULL, MS_BIND | MS_RDONLY, NULL, LABEL_MOUNT, &tmp_err);
      if (LIKELY (ret >= 0))
        return ret;

      if (tmp_err != NULL)

I think probably the line

      if (tmp_err != NULL)

can be removed.

I'm not suggesting changing the error handling convention used in crun. I just think it would be good to document how error handling in crun works.

[giuseppe]

perhaps we can simplify it.

What do you think of something like return any_error (err1, err2, ....); that returns the first error that is not NULL and frees the other ones?

[eriksjolund]

I'm not sure I understand the idea you have. How should any_error () be used? Is your idea related to the situations where an error is replaced? For instance

https://github.com/containers/crun/blob/92977c0fc843e4649fe4611a97ba12b06cb5073f/src/libcrun/cgroup.c#L164-L165

https://github.com/containers/crun/blob/92977c0fc843e4649fe4611a97ba12b06cb5073f/src/libcrun/linux.c#L5482-L5483

[giuseppe]

yeah, something that could help with that pattern.

If we return the first error, then an auto cleanup attribute is enough for the tmp_err

[eriksjolund]

I opened a draft PR

• https://github.com/containers/crun/pull/1926

just to show what I had in mind.