Skip to content

Conversation

@kotopesutility
Copy link

IMHO, it might be useful for users to get a list of hashing algorithms supported by the current build. I decided to use a dictionary-like structure so that users could match the algorithm name with the corresponding prefix.

So, I think, user can iterate over this list until they get {NULL, NULL}:
altlinux/pyxcrypt@c56c9db#diff-43dec635b9590d1a1e5874610e291500ffc204b411a10313a60d099106452f47R126

But they will have to check if the linked version of libxcrypt is higher than some value (where this list was implemented). Or libxcrypt can also provide some var to check with #ifdef

@besser82 besser82 requested review from besser82 and ldv-alt November 11, 2025 20:18
@codecov
Copy link

codecov bot commented Nov 11, 2025

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.07%. Comparing base (c1f37c1) to head (eee30ec).
⚠️ Report is 3 commits behind head on develop.

Files with missing lines Patch % Lines
lib/crypt.c 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #217      +/-   ##
===========================================
- Coverage    90.12%   90.07%   -0.05%     
===========================================
  Files           36       36              
  Lines         3767     3769       +2     
  Branches       739      739              
===========================================
  Hits          3395     3395              
- Misses         238      240       +2     
  Partials       134      134              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@besser82
Copy link
Owner

Thank you for your PR!

After having a first look at this there are some things I'd like to change:

  • Having a static array of supported hash algorithms in the crypt.h header seems a bit problematic, as that solution is not portable in such a way, that you cannot rely on this array to be accurate on run-time. Let say you build your application against a build of libxcrypt that supports glibc,strong,alt, one cannot simply replace the libxcrypt library with another build that supoorts glibc,strong,debian for instance. The application does not know about such a change without being rebuilt.
  • I'd favor a runtime evaluation with a function that returns a pointer to such an array, which is built into libxcrypt. Ideally such a function would be something like: const hash_method * crypt_get_supported_hash_methods (size_t *num_methods). The presense of this function could be signaled in crypt.h like #define CRYPT_GET_SUPPORTED_HASH_METHODS_AVAILABLE 1.
  • Such a function (and the underlying static array) should live inside its own compilation unit, so users of a static archive don't need to link it into their application, if it is not needed by their code.
  • A good place to put the macro definition for the contents of the array would be crypt-hashes.h.

It would be nice if you could make these changes to your PR, and I'll review again.

@vt-alt
Copy link
Collaborator

vt-alt commented Nov 12, 2025

Perhaps, num_methods is not needed, because array is static. But it would not hurt either. Also, as an alternative idea, return value could be a string (of some defined, extensible format) instead of binary array. Such as, "name1,prefix1;name2,prefix2;...".

@kotopesutility
Copy link
Author

Also, as an alternative idea, return value could be a string (of some defined, extensible format) instead of binary array. Such as, "name1,prefix1;name2,prefix2;...".

It makes user to parse the output of this function. IMHO, we have to do it inside this function.

@besser82
Copy link
Owner

Perhaps, num_methods is not needed, because array is static. But it would not hurt either. Also, as an alternative idea, return value could be a string (of some defined, extensible format) instead of binary array. Such as, "name1,prefix1;name2,prefix2;...".

Also, as an alternative idea, return value could be a string (of some defined, extensible format) instead of binary array. Such as, "name1,prefix1;name2,prefix2;...".

It makes user to parse the output of this function. IMHO, we have to do it inside this function.

I'm fine with either way, it can be a char * as long as we use a format that is easy to understand and fast to parse for the consumer, it can be some static binary array as well.

@kotopesutility
Copy link
Author

Perhaps, num_methods is not needed, because array is static. But it would not hurt either. Also, as an alternative idea, return value could be a string (of some defined, extensible format) instead of binary array. Such as, "name1,prefix1;name2,prefix2;...".

Also, as an alternative idea, return value could be a string (of some defined, extensible format) instead of binary array. Such as, "name1,prefix1;name2,prefix2;...".

It makes user to parse the output of this function. IMHO, we have to do it inside this function.

I'm fine with either way, it can be a char * as long as we use a format that is easy to understand and fast to parse for the consumer, it can be some static binary array as well.

I decided @vt-alt's solution is better. This evening, I'll rewrite the parsing of const char *crypt_get_supported_hash_methods output in pyxcrypt as a usage example.

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.

3 participants