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

core: handle ENOMEM gracefully during logging #5536

Merged
merged 1 commit into from
Mar 8, 2023

Conversation

bfaccini
Copy link
Contributor

@bfaccini bfaccini commented Feb 13, 2023

In order to survive and get an explicit message upon ENOMEM situation during logging, use a static generic message.

Ref: pmem/issues #5515
Signed-off-by: Bruno Faccini bruno.faccini@intel.com


This change is Reviewable

Copy link
Member

@pbalcer pbalcer left a comment

Choose a reason for hiding this comment

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

Would be useful to test this. There's a support for mocks and failure injection in our test framework, so testing malloc failures should be doable.

src/core/out.c Outdated
if (errormsg == NULL)
FATAL("!malloc");
if (errormsg == NULL) {
ERR("!malloc");
Copy link
Member

Choose a reason for hiding this comment

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

this will recurse I think, just return NULL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, sorry, looks like I forgot what the original problem is... :-(

src/core/out.c Outdated
@@ -425,6 +428,11 @@ out_error(const char *file, int line, const char *func,

char *errormsg = (char *)out_get_errormsg();

if (errormsg == NULL) {
ERR("There's no memory to properly format error strings.");
Copy link
Member

Choose a reason for hiding this comment

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

Same problem. Here I think you can just do Print("There's no memory to properly format error strings.");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @bfaccini)

@bfaccini bfaccini requested review from pbalcer and lukaszstolarczuk and removed request for pbalcer and lukaszstolarczuk February 14, 2023 18:59
Copy link
Member

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @bfaccini and @pbalcer)


-- commits line 2 at r3:
I guess you can squash your commits to just have the single one


src/core/out.c line 2 at r3 (raw file):

// SPDX-License-Identifier: BSD-3-Clause
/* Copyright 2014-2021, Intel Corporation */

Since you're using an intel e-mail address for your commits, you have to bump the copyright's date: 2014-2023

@lukaszstolarczuk
Copy link
Member

och, I guess you can also un-draft this PR...?

@bfaccini bfaccini marked this pull request as ready for review February 16, 2023 08:21
In order to survive and get an explicit message upon ENOMEM
situation during logging, use a static generic message.

Ref: pmem/issues#5515
Signed-off-by: Bruno Faccini <bruno.faccini@intel.com>
@bfaccini
Copy link
Contributor Author

I guess you can squash your commits to just have the single one

Done

Since you're using an intel e-mail address for your commits, you have to bump the copyright's date: 2014-2023

Done

@bfaccini
Copy link
Contributor Author

och, I guess you can also un-draft this PR...?

Done

@codecov
Copy link

codecov bot commented Feb 16, 2023

Codecov Report

Merging #5536 (0a8add6) into master (3c623ae) will decrease coverage by 0.01%.
The diff coverage is 25.00%.

@@            Coverage Diff             @@
##           master    #5536      +/-   ##
==========================================
- Coverage   71.62%   71.61%   -0.01%     
==========================================
  Files         162      162              
  Lines       24256    24259       +3     
==========================================
  Hits        17373    17373              
- Misses       6883     6886       +3     

Copy link
Member

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

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

thx

👍

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pbalcer)

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pbalcer)

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pbalcer)

@wlemkows wlemkows merged commit 9b201f7 into pmem:master Mar 8, 2023
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.

5 participants