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.

Sign up for GitHub

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

Jump to bottom

Fix fedora rawhide build, part2. #657

Merged
merged 1 commit into from
Feb 17, 2018
Merged

Fix fedora rawhide build, part2. #657

merged 1 commit into from
Feb 17, 2018

Conversation

simonmichal
Copy link
Contributor

A couple of fixes for Fedora rawhide, GCC 8

abh3
abh3 reviewed Feb 9, 2018
View reviewed changes
src/XrdCms/XrdCmsRRQ.hh
rdFast = 0;
rdSlow = 0;
}

Copy link
Member

Choose a reason for hiding this comment

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

What is the compiler complaining about here that makes you change all inline initialization to a function call? I am concerned that this structure may defeat compiler optimization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I add the 'Reset' method is here:

60da79d#diff-bffded318632dc8e33cbdb6c4dcc1d34L134

In XrdCmsRRQ.cc the compiler was complaining that an object with a non-trivial constructor is being initialized with memset. As the constructor does basically the same as 'Reset' I though that implementing the constructor in terms of 'Reset' method would simplify the code. IMHO the compiler will be smart enough to first inline 'Reset' and then the constructor. That said, I don't have any strong feelings on this one, I can revert to the old constructor implementation if you wish (as long as the 'Reset' stays).

abh3
abh3 reviewed Feb 9, 2018
View reviewed changes
src/XrdDig/XrdDigAuth.cc
// Clear the arec the fastest way possible
//
memset(aEnt.eP, 0, sizeof(XrdDigAuthEnt));

Copy link
Member

Choose a reason for hiding this comment

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

Uh, this doesn't look kosher. You just eliminated initializing aEnt so I doubt his will work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, the compiler was complaining that an object with a non-trivial constructor is being initialized with memset.

Now the fix rational:

aEnt.eP is a newly created instance of 'XrdDigAuthEnt':
https://github.com/xrootd/xrootd/blob/fedora-rawhide-fix/src/XrdDig/XrdDigAuth.cc#L227

note that I modified the default constructor of 'XrdDigAuthEnt' :
60da79d#diff-a726e008bf733c6a23ce19da88801122R56

now, it initializes all the members to 0, and clears member arrays, so it is equivalent to the memset I removed.

Copy link
Member

Choose a reason for hiding this comment

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

OK, got it.

abh3
abh3 reviewed Feb 9, 2018
View reviewed changes
src/XrdSec/XrdSecEntity.hh
rsvd(0), addrInfo(0), tident(""), sessvar(0)
{strncpy(prot, pName, XrdSecPROTOIDSIZE-1);
{Reset();
strncpy(prot, pName, XrdSecPROTOIDSIZE-1);
prot[XrdSecPROTOIDSIZE-1] = '\0';
Copy link
Member

Choose a reason for hiding this comment

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

How does this square with ABI compatability? The in-lining differs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code compiled against the old version of constructor will have the old version of the constructor inlined, there's no ABI compatibility problem. The new and the old version of the constructor are functionally equivalent so it is OK.

Copy link
Member

Choose a reason for hiding this comment

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

Well, that's only true if the Reset() is actually inlined, isn't it?But I guess we are not providing regressed compatibility (i.e. new code running with old libraries).

abh3
abh3 reviewed Feb 9, 2018
View reviewed changes
src/XrdSsi/XrdSsiResponder.cc
@@ -207,7 +207,7 @@ void XrdSsiResponder::ReleaseRequestBuffer()

XrdSsiResponder::Status XrdSsiResponder::SetMetadata(const char *buff, int blen)
{
XrdSsiMutexMon(spMutex);
XrdSsiMutexMon lck(spMutex);

// If we don't have a request or the args are invalid, return an error.
//
Copy link
Member

Choose a reason for hiding this comment

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

You may get merge conflicts here if you didn't use the latest githead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did rebase against the latest git HEAD :-) so no conflicts :-)

abh3
abh3 reviewed Feb 9, 2018
View reviewed changes
Copy link
Member

@abh3 abh3 left a comment

Choose a reason for hiding this comment

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

Well, that's a handful. Please review my comments before merging. I suspect this will be ongoing battle until we switch to this compiler.

gganis
gganis reviewed Feb 12, 2018
View reviewed changes
src/XrdSeckrb5/XrdSecProtocolkrb5.cc
@@ -541,7 +541,7 @@ int XrdSecProtocolkrb5::Authenticate(XrdSecCredentials *cred,
int XrdSecProtocolkrb5::Init(XrdOucErrInfo *erp, char *KP, char *kfn)
{
krb_rc rc;
char buff[1024];
char buff[2048];
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, why 2048 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem was that in here:

https://github.com/xrootd/xrootd/blob/fedora-rawhide-fix/src/XrdSeckrb5/XrdSecProtocolkrb5.cc#L585

"Unable to start sequence on the keytab file " gets written to buff, plus 'krb_kt_name', which is up to 1024 (at least that's what the compiler thinks), so the compiler was complaining that 'buff' might be too short. I just picked the next round number ;-), it could have been shorter.

@gganis
Copy link
Member

gganis commented Feb 12, 2018

The changes in the parts I am concerned more directly look fine, as far as I can see.
I have seen that are a number of hardcoded 1024 becoming hardcoded 2048 ... perhaps we could define a generic buffer length parameter and used that consistently?

@simonmichal simonmichal force-pushed the fedora-rawhide-fix branch 3 times, most recently from 98fbc29 to 9bf7e8a Compare February 16, 2018 09:33
@simonmichal
Copy link
Contributor Author

@gganis : in general I agree that having a constance would be better, but for now we lack a common header with global constance, also re-factoring hardcoded buffer sizes in this PR will decrease readability. So I would decouple this kind of re-factoring from this PR ;-)

@simonmichal simonmichal merged commit d2d78bf into master Feb 17, 2018
@simonmichal simonmichal deleted the fedora-rawhide-fix branch May 28, 2018 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gganis gganis gganis left review comments

@abh3 abh3 abh3 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@simonmichal @gganis @abh3