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

common: Correction of cache_req debug string ID format #448

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@amitkumar50
Contributor

amitkumar50 commented Nov 14, 2017

The cache-req debug string representation uses a wrong format
specifier for by-ID requests.
%d should be replaced with %"SPRIgid".

Resolves: https://pagure.io/SSSD/sssd/issue/3570

@centos-ci

This comment has been minimized.

Show comment
Hide comment
@centos-ci

centos-ci Nov 14, 2017

Collaborator

Can one of the admins verify this patch?

Collaborator

centos-ci commented Nov 14, 2017

Can one of the admins verify this patch?

@centos-ci

This comment has been minimized.

Show comment
Hide comment
@centos-ci

centos-ci Nov 14, 2017

Collaborator

Can one of the admins verify this patch?

Collaborator

centos-ci commented Nov 14, 2017

Can one of the admins verify this patch?

@fidencio

This comment has been minimized.

Show comment
Hide comment
@fidencio

fidencio Nov 14, 2017

Contributor

Ack! I'll fire an internal CI run just for the sake of the process and add the "Accepted" label when I get the result.

Thanks for the contribution, @amitkumar50!

Contributor

fidencio commented Nov 14, 2017

Ack! I'll fire an internal CI run just for the sake of the process and add the "Accepted" label when I get the result.

Thanks for the contribution, @amitkumar50!

@fidencio

This comment has been minimized.

Show comment
Hide comment
@fidencio

fidencio Nov 14, 2017

Contributor

ok to test

Contributor

fidencio commented Nov 14, 2017

ok to test

@amitkumar50

This comment has been minimized.

Show comment
Hide comment
@amitkumar50

amitkumar50 Nov 16, 2017

Contributor

Are some changes required? Label is 'Changed Requested'

Contributor

amitkumar50 commented Nov 16, 2017

Are some changes required? Label is 'Changed Requested'

@fidencio

This comment has been minimized.

Show comment
Hide comment
@fidencio

fidencio Nov 16, 2017

Contributor

@amitkumar50, you made the changes addressing only half of @lslebodn's comments.

@lslebodn said:
"This one is for UID. So it should be SPRIuid ..."

Okay, it's fixed by your current patches ...

"But typ of data->id is uint32_t and not gid_t/uid_t/id_t.
So please check man inttypes.h for right format string for uint32_t"

This is not fixed. Although it seems nitpicking to me as gid_t, uid_t and id_t as all of those ended up being defined as u32 type anyways ... all of those should use PRIu32 in order to address @lslebodn's comment.

So, please, re-update the patches addressing @lslebodn's comments. (Adding back the "Changes Requested" label).

Contributor

fidencio commented Nov 16, 2017

@amitkumar50, you made the changes addressing only half of @lslebodn's comments.

@lslebodn said:
"This one is for UID. So it should be SPRIuid ..."

Okay, it's fixed by your current patches ...

"But typ of data->id is uint32_t and not gid_t/uid_t/id_t.
So please check man inttypes.h for right format string for uint32_t"

This is not fixed. Although it seems nitpicking to me as gid_t, uid_t and id_t as all of those ended up being defined as u32 type anyways ... all of those should use PRIu32 in order to address @lslebodn's comment.

So, please, re-update the patches addressing @lslebodn's comments. (Adding back the "Changes Requested" label).

@lslebodn

This comment has been minimized.

Show comment
Hide comment
@lslebodn

lslebodn Nov 16, 2017

Contributor

This is not fixed. Although it seems nitpicking to me as gid_t, uid_t and id_t as all of those ended up being defined as u32 type anyways ... all of those should use PRIu32 in order to address @lslebodn's comment.

It is not nitpick. Some platform have 64 bit id_t. Therefore we have custom string formats for
gid_t, uid_t and id_t And you should not use PRIu32 for formatting 64 bit integers.
Therefore we need to use the format string which is tight to type (uint32_t) in this case.

Contributor

lslebodn commented Nov 16, 2017

This is not fixed. Although it seems nitpicking to me as gid_t, uid_t and id_t as all of those ended up being defined as u32 type anyways ... all of those should use PRIu32 in order to address @lslebodn's comment.

It is not nitpick. Some platform have 64 bit id_t. Therefore we have custom string formats for
gid_t, uid_t and id_t And you should not use PRIu32 for formatting 64 bit integers.
Therefore we need to use the format string which is tight to type (uint32_t) in this case.

@amitkumar50

This comment has been minimized.

Show comment
Hide comment
@amitkumar50

amitkumar50 Nov 16, 2017

Contributor

@fidencio Thanks I read inttypes.h convention here goes PRIuN.
Done.

Contributor

amitkumar50 commented Nov 16, 2017

@fidencio Thanks I read inttypes.h convention here goes PRIuN.
Done.

@fidencio

This comment has been minimized.

Show comment
Hide comment
@fidencio

fidencio Nov 16, 2017

Contributor

@lslebodn, hmm. makes sense, thanks for the explanation.

Seems that @amitkumar50 new PR is okay with your suggestion, but I'd like to hear it from you.

Contributor

fidencio commented Nov 16, 2017

@lslebodn, hmm. makes sense, thanks for the explanation.

Seems that @amitkumar50 new PR is okay with your suggestion, but I'd like to hear it from you.

@lslebodn

This comment has been minimized.

Show comment
Hide comment
@lslebodn

lslebodn Nov 16, 2017

Contributor

master:

Contributor

lslebodn commented Nov 16, 2017

master:

@lslebodn lslebodn closed this Nov 16, 2017

@lslebodn lslebodn added the Pushed label Nov 16, 2017

@jhrozek

This comment has been minimized.

Show comment
Hide comment
@jhrozek

jhrozek Nov 16, 2017

Contributor

btw which platforms use 64bit id_t?

Contributor

jhrozek commented Nov 16, 2017

btw which platforms use 64bit id_t?

common: Correction of cache_req debug string ID format
The cache-req debug string representation uses a wrong format
specifier for by-ID requests.
data->id (uint32_t) should be replaced with  %"PRIu32"
in cache_req_group_by_id.c, cache_req_object_by_id.c &
cache_req_user_by_id.c.

Resolves: https://pagure.io/SSSD/sssd/issue/3570
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment