Skip to content

libct/nsenter: ignore unused return value of write#3154

Closed
kailun-qin wants to merge 1 commit intoopencontainers:masterfrom
kailun-qin:fix-ignore-return
Closed

libct/nsenter: ignore unused return value of write#3154
kailun-qin wants to merge 1 commit intoopencontainers:masterfrom
kailun-qin:fix-ignore-return

Conversation

@kailun-qin
Copy link
Copy Markdown
Contributor

@kailun-qin kailun-qin commented Aug 12, 2021

A warning (warn_unused_result) may show up during compilation if a
function returns a result that is never used.

nsexec.c: In function ‘write_log’:
nsexec.c:165:2: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result]
  165 |  write(logfd, json, ret);
      |  ^~~~~~~~~~~~~~~~~~~~~~~

This patch explicitly disables/ignores this "-Wunused-result" warning
for write() by using gcc diagnostic pragmas.

Signed-off-by: Kailun Qin kailun.qin@intel.com

Comment on lines +167 to +168
if (write(logfd, json, ret) != ret)
goto out;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is useless...

A way to suppress a warning is (void)write(...)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Simply casting to (void) won't work. See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=25509 for detailed info.

Alternatives below if we really don't want to check the return value, which may however lose kind of readability:

  1. (void)!write(...)
  2. if (write(...));

What's the preference? Thanks! @kolyshkin

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh my...

Frankly, I don't know. What's the best way from the readability point of view?

Perhaps something like

if (write(logfd, json, ret) < 0)
	; // nothing we can do here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated. PTAL, thanks! @kolyshkin

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess that if stmt may break another linter.

Isn't there an gcc __attribute__ to ignore the warning? (I'm googling around...)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

#pragma GCC diagnostic ignored "-Wunused-result" may work (untested)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@AkihiroSuda Thanks for the comment!

emm yes, I agree this may break another linter. But #pragma GCC diagnostic ignored "-Wunused-result" won't work for older gcc versions before Diagnostic Pragmas was introduced.

Do you think adding braces to avoid linters checking empty bodies for if statements work for you?

if (write(logfd, json, ret) < 0) {
	; // nothing we can do here
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated. PTAL thanks! @kolyshkin @AkihiroSuda

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Even gcc4 seems supporting that pragma https://stackoverflow.com/questions/63229590/enabling-long-long-in-c89-at-gcc-3-2-4-4-and-5-4

So we can safely use the pragma

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@AkihiroSuda Thanks for checking this!

Updated using pragmas, PTAL. Thanks!

@kailun-qin kailun-qin force-pushed the fix-ignore-return branch 4 times, most recently from 229cdd0 to ec1aba1 Compare August 13, 2021 01:35
kolyshkin
kolyshkin previously approved these changes Aug 13, 2021
Copy link
Copy Markdown
Contributor

@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.

LGTM

@kailun-qin
Copy link
Copy Markdown
Contributor Author

@AkihiroSuda @cyphar Would you please kindly take a look when you get a chance? Thanks!

Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

@kolyshkin
Copy link
Copy Markdown
Contributor

The alternative is #3165.

@kailun-qin kailun-qin changed the title libct/nsenter: fix unchecked return value of write libct/nsenter: ignore unused return value of write Aug 17, 2021
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wunused-result"
write(logfd, json, ret);
#pragma GCC diagnostic pop
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks way too heavy to silent a single warning.

I think something like #3168 looks more elegant; PTAL.

A warning (warn_unused_result) may show up during compilation if a
function returns a result that is never used.

This patch explicitly disables/ignores this "-Wunused-result" warning
for write() by using gcc diagnostic pragmas.

Signed-off-by: Kailun Qin <kailun.qin@intel.com>
@AkihiroSuda
Copy link
Copy Markdown
Member

Fixed in #3168

Thanks anyway for contribution

@kailun-qin kailun-qin deleted the fix-ignore-return branch August 18, 2021 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants