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.

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

Improve const correctness for P11KitUri #152

Merged
merged 1 commit into from May 24, 2018
Merged

Conversation

npmccallum
Copy link
Contributor

This does not improve const for the getters. The reason for this is that
they are usually passed into the PKCS#11 APIs directly and these APIs
are not const correct. Trying to force const correctnesss here would
result in pain for library consumers.

This is an API and ABI compatible change.

@ueno
Copy link
Member

ueno commented May 22, 2018

Thank you. But actually, adding const to the public interface seems to affect the ABI:

$ abidiff /usr/lib64/libp11-kit.so .libs/libp11-kit.so
Functions changes summary: 0 Removed, 5 Changed (1 filtered out), 0 Added functions
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

5 functions with some indirect sub-type change:

  [C]'function const char* p11_kit_uri_get_pin_source(P11KitUri*)' at uri.c:690:1 has some indirect sub-type changes:
    parameter 1 of type 'P11KitUri*' changed:
      in pointed to type 'typedef P11KitUri':
        entity changed from 'typedef P11KitUri' to 'const P11KitUri'
        type size hasn't changed
...

I suppose these are harmless, but I am not completely sure. @nmav do you happen to have an experience of applying such changes?

@npmccallum
Copy link
Contributor Author

@nmav
Copy link
Contributor

nmav commented May 22, 2018

I also think that const-addition changes don't affect ABI. abi-compliance-checker may be more explicit about it.

const char *pin);

const char* p11_kit_uri_get_pin_source (P11KitUri *uri);
const char* p11_kit_uri_get_pin_source (const P11KitUri *uri);
Copy link
Member

Choose a reason for hiding this comment

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

Should we add const to other const char * getters, such as p11_kit_uri_get_module_name() and p11_kit_uri_get_module_path()?

This does not improve const for the getters. The reason for this is that
they are usually passed into the PKCS#11 APIs directly and these APIs
are not const correct. Trying to force const correctnesss here would
result in pain for library consumers.

This is an API and ABI compatible change.
@npmccallum
Copy link
Contributor Author

@ueno I have added const to the following functions:

  • p11_kit_uri_get_pinfile()
  • p11_kit_uri_get_module_name()
  • p11_kit_uri_get_module_path()
  • p11_kit_uri_get_vendor_query()

I believe this now covers all of the low hanging fruit.

@ueno ueno merged commit e42dcf5 into p11-glue:master May 24, 2018
@ueno
Copy link
Member

ueno commented May 24, 2018

Thank you!

@ueno ueno added this to the 0.23.11 milestone May 24, 2018
@asavah
Copy link

asavah commented May 25, 2018

This breaks building gnutls 3.6.2 with p11-kit

In file included from ./../pkcs11_int.h:34:0,
                 from x509.c:37:
/home/asavah/kross/build/rpi3/rootfs/usr/include/p11-kit-1/p11-kit/uri.h:99:68: error: unknown type name 'CK_INFO'
                                                              const CK_INFO *info);
                                                                    ^~~~~~~
/home/asavah/kross/build/rpi3/rootfs/usr/include/p11-kit-1/p11-kit/uri.h:104:68: error: unknown type name 'CK_SLOT_INFO'
                                                              const CK_SLOT_INFO *slot_info);
                                                                    ^~~~~~~~~~~~
/home/asavah/kross/build/rpi3/rootfs/usr/include/p11-kit-1/p11-kit/uri.h:113:68: error: unknown type name 'CK_TOKEN_INFO'
                                                              const CK_TOKEN_INFO *token_info);
                                                                    ^~~~~~~~~~~~~
/home/asavah/kross/build/rpi3/rootfs/usr/include/p11-kit-1/p11-kit/uri.h:134:68: error: unknown type name 'CK_ATTRIBUTE'
                                                              const CK_ATTRIBUTE *attrs,
                                                                    ^~~~~~~~~~~~
Makefile:1512: recipe for target 'x509.lo' failed

@nmav
Copy link
Contributor

nmav commented May 26, 2018

Ouch, indeed this patch also changes the types while adding the const. It seems that the block inside #ifdef CRYPTOKI_GNU has to be updated as well for the new types used. It seems that the testsuite doesn't compile any program with this definition on. Changing test-uri.c to define that, catches the issue.

npmccallum added a commit to npmccallum/p11-kit that referenced this pull request May 27, 2018
Since we now use the non-PTR variants in function definitions, we need
to ensure that we properly typedef them if CRYPTOKI_GNU is set.

See: p11-glue#152
npmccallum added a commit to npmccallum/p11-kit that referenced this pull request May 27, 2018
Since we now use the non-PTR variants in function definitions, we need
to ensure that we properly typedef them if CRYPTOKI_GNU is set.

See: p11-glue#152
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants