Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

container, error: update error handling #1659

Merged
merged 1 commit into from
Feb 11, 2025

Conversation

eriksjolund
Copy link
Contributor

@eriksjolund eriksjolund commented Feb 7, 2025

What is the preferred way to write a check that indicates that a failure is not expected and that such a failure would indicate a programming errror?

One way is to write assert() (but such a check is only active for an executable that was compiled with the debug option enabled)

For example:

  assert(err != NULL);
  assert(*err != NULL);

This is another way:

if (err == NULL || *err == NULL) {
  // programming error
  _exit(EXIT_FAILURE);
}

I prefer assert() slightly more.

@eriksjolund eriksjolund force-pushed the update-error-handling branch 3 times, most recently from ec1b7b3 to b9e033d Compare February 7, 2025 12:04
@@ -70,8 +72,14 @@ crun_error_wrap (libcrun_error_t *err, const char *fmt, ...)
char *swap;
int ret;

if (err == NULL || *err == NULL)
return 0;
assert (err != NULL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the rest of the code, we used something like return crun_make_error (err, 0, "internal error");

because there is the risk that an assert would produce output that is ignored (when setting up the child process)

@eriksjolund eriksjolund force-pushed the update-error-handling branch from b9e033d to ec0c304 Compare February 11, 2025 07:03
@eriksjolund
Copy link
Contributor Author

I pushed a new version that makes less changes.

Right now

    {
      // Internal error
      return 0;
    }

Should maybe be

    {
      // Internal error
      return -1;
    }

to make it more robust. I'm not sure what's best.

@eriksjolund eriksjolund marked this pull request as ready for review February 11, 2025 07:07
Signed-off-by: Erik Sjölund <erik.sjolund@gmail.com>
@eriksjolund eriksjolund force-pushed the update-error-handling branch from ec0c304 to 90a321c Compare February 11, 2025 07:33
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@giuseppe giuseppe merged commit bb7c4fe into containers:main Feb 11, 2025
48 checks passed
@eriksjolund eriksjolund deleted the update-error-handling branch February 12, 2025 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants