Adjust logic for compat shims. #11
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
With great difficulty, I have signed the CLA. I'd consider it a bug that I can't change which of my identities is being used to sign the CLA like I can with almost any other Google property... |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
Thanks for jumping through those administrative hoops ,and thanks for the contribution! |
|
Any time! I must admit though I think there may be a bug in my PR. I've observed an intermittent hang in NSS queries this afternoon which happens on around 1% of the fleet I manage. At this time I am unsure if the hang is in libnss-cache or in musl-nscd (the compat layer for NSS on musl). If the bug is in libnss-cache, it is certainly in the value I patched in for I'm also in conversation with the devs over in #musl, and I'll update this PR with a comment once I track down where the bug is once and for all. |
|
Great, thanks for the update. I'll hold off on a version bump for now.
…On Fri, 26 Jul 2019 at 14:47, Michael Aldridge ***@***.***> wrote:
Any time! I must admit though I think there may be a bug in my PR. I've
observed an intermittent hang in NSS queries this afternoon which happens
on around 1% of the fleet I manage. At this time I am unsure if the hang is
in libnss-cache or in musl-nscd (the compat layer for NSS on musl). If the
bug is in libnss-cache, it is certainly in the value I patched in for
ALIGNBYTES on Linux.
I'm also in conversation with the devs over in #musl, and I'll update this
PR with a comment once I track down where the bug is once and for all.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#11?email_source=notifications&email_token=AAXFX6Y7SRCMU2NI4HJ4EW3QBKFWJA5CNFSM4IGTCLBKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD23SCHA#issuecomment-515318044>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAXFX62FKY6275L2ZPFMX2LQBKFWJANCNFSM4IGTCLBA>
.
|
Can you give me some more information about this? E.g. describe the sequence of actions that occurred and what went wrong, please. |
|
Sure, I'm normally signed in with at least 6 Google identities on Chrome. On most Google properties, there is a selector in the upper right hand corner where the I can switch what account is being used as the identity for the page. On the site for the CLA, only one ID was selected and no mechanism that I could find was provided to switch. I had to open a private browsing window and sign in just the one identity, then go through the CLA signing flow to get it to attach to the right context (I try to keep it relatively clean what's attached to each ID since some are for other organizations). Feel free to email me at the email listed on my GitHub account and I can provide more detailed information. |
|
Great, thanks for the info. I've escalated the report and will let you know if we need more info. |
|
@jaqx0r The bug was isolated to another library. Here is the issue for the problem I was observing: pikhq/musl-nscd#8. Feel free to cut a release here at your convenience, I will update downstream packages for Void once you have done so. |
|
Thankyou!
…On Sat, 27 Jul 2019 at 06:53, Michael Aldridge ***@***.***> wrote:
@jaqx0r <https://github.com/jaqx0r> The bug was isolated to another
library. Here is the issue for the problem I was observing:
pikhq/musl-nscd#8 <pikhq/musl-nscd#8>. Feel free
to cut a release here at your convenience, I will update downstream
packages for Void once you have done so.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#11?email_source=notifications&email_token=AAXFX64QHRUWDSIOYED6J6TQBNW5XA5CNFSM4IGTCLBKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD25ZQBQ#issuecomment-515610630>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAXFX64TNB4E6ND6RWDJE23QBNW5XANCNFSM4IGTCLBA>
.
|
| // defined above that guard the rest of the compat layer. On Linux we | ||
| // don't pull in param.h as it is very obsolete. | ||
| #include <stdint.h> | ||
| #define ALIGNBYTES _Alignof(max_align_t) |
There was a problem hiding this comment.
This type requires C11 which isn't a real problem but the symbols might not be visible depending on the system. What do you think about removing the ifdefs and param.h include altogether and doing this:
#define ALIGNBYTES (sizeof(uintptr_t) - 1)
There was a problem hiding this comment.
I had considered the C11 issue when drafting this patch but thought it shouldn't be an issue on FreeBSD or most modern *nix's. I'm all for removing param.h dependencies, so if you are confident that changing from the _Alignof won't affect any currently working target I'd say go ahead and send the PR.
This PR adjusts the logic that chooses when the compat shims get built. The immediate use case is to permit the use of libnss_cache with the excellent musl C library on Linux. I've made what I believe to be the minimal set of changes that will work here, but there is a hazy line around the patch I have to replace the use of sys/param.h on Linux.
A special thanks to @Vaelatern for assisting with debugging while working on this PR.