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

[eglib] Fix g_strerror / monoeg_g_strerror #5323

Merged
merged 1 commit into from Aug 10, 2017

Conversation

Projects
None yet
4 participants
@cherusker
Contributor

cherusker commented Aug 7, 2017

gstr.c: In function ‘monoeg_g_strerror’:
gstr.c:238:13: warning: assignment makes integer from pointer without a cast [-Wint-conversion]
   while ((r = strerror_r (errnum, buff, buff_len - 1))) {
             ^

This warning caught my eye when I compiled Mono this morning. The current code is not harmful but, if I am not mistaken, it can issue invalid error messages: g_strdup_printf ("Invalid Error code '%d'", errnum) will be used for every errnum on platforms that use the "GNU-specific" version of strerror_r (like my Fedora 26 machine).

@dnfclas

This comment has been minimized.

dnfclas commented Aug 7, 2017

@cherusker,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@akoeplinger

This comment has been minimized.

Member

akoeplinger commented Aug 7, 2017

build

@luhenry luhenry requested a review from kumpera Aug 7, 2017

@@ -38,6 +38,12 @@
#include <errno.h>
#if defined (_POSIX_C_SOURCE) && defined (_GNU_SOURCE)
#if (_POSIX_C_SOURCE >= 200112L) && !_GNU_SOURCE
#define HAVE_STRERROR_R_XSI

This comment has been minimized.

@kumpera

kumpera Aug 7, 2017

Member

HAVE_xxxx macros should only be defined by configure.

It's ok to check it here, but use a different name prefix, like USE_xxx.

Additionally, there should be a comment on why this is being checked.

@@ -251,10 +258,16 @@ g_strerror (gint errnum)
error_messages [errnum] = g_strdup (buff);
if (buff != tmp_buff)
g_free (buff);
#else
#else /* HAVE_STRERROR_R_XSI */
buff = strerror_r (errnum, buff, buff_len);

This comment has been minimized.

@kumpera

kumpera Aug 7, 2017

Member

If we are to trust the docs, this is ok, as buff would alternatively point to an immutable buffer.

This comment has been minimized.

@cherusker

cherusker Aug 7, 2017

Contributor

Usually, I trust the man pages - I guess that's a safe bet? In addition, it also makes a lot of sense that these strings are immutable; the custom string buffer will only be used for personalised messages.

@kumpera

This comment has been minimized.

Member

kumpera commented Aug 7, 2017

There are some minor changes in the code, plus it would be nice to expand the commit message to include further explanation and pointers.

@cherusker cherusker force-pushed the cherusker:cherusker-2017-08-07-fix-strerror branch 2 times, most recently from 11f8b87 to fc86f76 Aug 8, 2017

@cherusker

This comment has been minimized.

Contributor

cherusker commented Aug 10, 2017

@kumpera I addressed all your thoughts - could you please have another look? :)

@kumpera

This comment has been minimized.

Member

kumpera commented Aug 10, 2017

LGTM but we need to evaluate the test failures before merging

@cherusker

This comment has been minimized.

Contributor

cherusker commented Aug 10, 2017

I will quickly rebase the PR (which might fix the issue with API Diff?) and will then try to rebuild all lanes until they are green; @akoeplinger just added this feature and I wanted to test it anyways :) As far as I understand, the current issues look unrelated to the changes.

Distinguish between the XSI-compliant and the GNU-specific version of…
… `strerror_r ()`

- add USE_STRERROR_R_XSI to use the feature test macros conveniently later
- deal with the GNU-specific version of `strerror_r ()` separately (leave the code for the XSI-compliant version untouched)
- make `buff_len` size_t as this is ISO compliant (even though it should not really matter in this case)

@cherusker cherusker force-pushed the cherusker:cherusker-2017-08-07-fix-strerror branch from fc86f76 to dffb0dd Aug 10, 2017

@cherusker

This comment has been minimized.

Contributor

cherusker commented Aug 10, 2017

@monojenkins build OS X i386

@cherusker

This comment has been minimized.

Contributor

cherusker commented Aug 10, 2017

@monojenkins build OS X x64

@cherusker

This comment has been minimized.

Contributor

cherusker commented Aug 10, 2017

@monojenkins build Windows i386

@cherusker

This comment has been minimized.

Contributor

cherusker commented Aug 10, 2017

@monojenkins build Windows x64

@cherusker

This comment has been minimized.

Contributor

cherusker commented Aug 10, 2017

@monojenkins build Linux ARMv5 soft float

@kumpera kumpera merged commit c695c1e into mono:master Aug 10, 2017

8 of 12 checks passed

OS X x64 Build finished. 45731 tests run, 1285 skipped, 1 failed.
Details
Windows x64 Build finished. 40367 tests run, 1113 skipped, 0 failed.
Details
Linux ARMv5 soft float Build started sha1 is merged.
Details
OS X i386 Build started sha1 is merged.
Details
API Diff No public API changes found.
Details
Linux AArch64 Build finished. 46971 tests run, 1408 skipped, 0 failed.
Details
Linux ARMv7 hard float Build finished. 46941 tests run, 1403 skipped, 0 failed.
Details
Linux i386 Build finished. 46974 tests run, 1400 skipped, 0 failed.
Details
Linux x64 Build finished. 46974 tests run, 1402 skipped, 0 failed.
Details
Linux x64 FullAOT Build finished. 20903 tests run, 528 skipped, 0 failed.
Details
Test Result Viewer Click to view aggregated test results (Xamarin internal).
Details
Windows i386 Build finished. 40434 tests run, 1111 skipped, 0 failed.
Details

@cherusker cherusker deleted the cherusker:cherusker-2017-08-07-fix-strerror branch Aug 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment