Skip to content

Introduce pwarnf() for better diagnosis of socket/fd write issues.#554

Merged
jnovy merged 1 commit intocontainers:mainfrom
jnovy:540
Apr 24, 2025
Merged

Introduce pwarnf() for better diagnosis of socket/fd write issues.#554
jnovy merged 1 commit intocontainers:mainfrom
jnovy:540

Conversation

@jnovy
Copy link
Collaborator

@jnovy jnovy commented Apr 16, 2025

Related: #540

@jnovy jnovy requested a review from Copilot April 16, 2025 07:52
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/utils.h:98

  • [nitpick] The syslog message in the pwarnf() macro uses '' which is inconsistent with the pwarnf() naming; consider updating it to '' or another consistent label.
syslog(LOG_INFO, "conmon %.20s <nwarn>: " fmt ": %s\n", log_cid, ##__VA_ARGS__, strerror(errno));

Copy link
Collaborator

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

Frankly,

  • since we already have #define _GNU_SOURCE in the code, I'd rather use nwarnf with added %m;
  • it's probably cleaner to use a ternary operator to add fd/socket to the message.

IOW something like this (untested):

...
if (w < 0) {
	pwarnf("Failed to write to %s %s: %m",
		local_sock->is_stream ? "fd" : "label",
		local_sock->label);
} else {
...

Finally, can ditch pwarn, replacing it with nwarnf(... ": %m") (but this is out of scope for this PR).

Related: containers#540

Signed-off-by: Jindrich Novy <jnovy@redhat.com>
@kolyshkin
Copy link
Collaborator

Opened #556 as an alternative to this one.

@jnovy
Copy link
Collaborator Author

jnovy commented Apr 24, 2025

Merging this one as following PRs are dependent on existence of nwarnf()

@jnovy jnovy merged commit 475c7de into containers:main Apr 24, 2025
27 of 29 checks passed
@jnovy jnovy deleted the 540 branch September 1, 2025 11:57
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.

3 participants