Fixed pid wrapping in sss_cli_check_socket #6616
Conversation
|
What is the difference with #6608? |
alexey-tikhonov
added
Waiting for review
no-backport
src/sss_client/common.c
Outdated
| sss_cli_close_socket(); | ||
| } | ||
| } | ||
| sss_cli_sd = -1; | ||
| mypid = getpid(); | ||
| ret = lstat("/proc/self/", &selfsb); |
There was a problem hiding this comment.
Hi,
you already have all the data in myself_sb so, not need to call lstat() again. And I think it would be good if you can to the same with the pid and avoid calling getpid() twice..
bye,
Sumit
There was a problem hiding this comment.
Hi,
My issue is: #6592
I do not know the use of mypid, but mypid causes this issue. so I imitate the writing method of mypid and add the selfsb and myself_sb to reduce the probability of problems caused by pid loopback.
There was a problem hiding this comment.
Hi,
yes, I understand this. However, currently getpid() and with your patch lstat() as well are called twice to get the same information. Since those are system-call both need some time to execute and since the second call is redundant it would help to avoid it.
The getpid() related code it quite old and at the time it was written it might have even been cheap because glibc had a pid cache which helped to avoid a system-call. But this changed are there is currently also no alternatives, e.g. as a vDSO call, available, see e.g. https://bugzilla.redhat.com/show_bug.cgi?id=1469757 and the links in the ticket for details. I wasn't aware of this until reviewing your patch as well and thought that since your patch touched this area of the code it can be improved as part of your patch.
If @alexey-tikhonov, agrees we can keep your patch as it is but open a new ticket to remove the second getpid() and lstat()` call in a separate PR. But feel free to do the change yourself.
bye,
Sumit
There was a problem hiding this comment.
Nice catch. I think at least 2nd lstat() can be avoided within this patch.
Also I think we should really consider per-transaction sockets, as you proposed in other ticket...
There was a problem hiding this comment.
thanks, I see. The redundant second call will be deleted.
…eck_socket() the second call to getpid() and lstat() is redundant.
|
Thanks, ACK. |
alexey-tikhonov
added
Accepted
Ready to push
Resolves: #6592