Re: Removal of plaintext password type references

On 05/10/2017 08:01 AM, Michael Paquier wrote:
> On Wed, May 10, 2017 at 10:51 AM, Vaishnavi Prabakaran
> <vaishnaviprabakaran(at)gmail(dot)com> wrote:
>> Following recent removal of support to store password in plain text,
>> modified the code to
>>
>> 1. Remove "PASSWORD_TYPE_PLAINTEXT" macro
>> 2. Instead of using "get_password_type" to retrieve the encryption method
>> AND to check if the password is already encrypted or not, modified the code
>> to
>> a. Use "get_password_encryption_type" function to retrieve encryption
>> method.
>> b. Use "isPasswordEncrypted" function to check if the password is already
>> encrypted or not.
>>
>> These changes are mainly to increase code readability and does not change
>> underlying functionality.
>
> Actually, this patch reduces readability.

Yeah, I don't think this is an improvement. Vaishnavi, did you find the
current code difficult? Perhaps some extra comments somewhere would help?

Also note that changing the signature check_password_hook_type would
break any external modules that use the hook. Removing
PASSWORD_TYPE_PLAINTEXT will do that too, because any password hook
function would use that constant (see e.g. contrib/passwordcheck). If we
were to change the signature, I'd actually like to simplify it by
removing the password_type parameter altogether. The hook function can
call get_password_type() on the password itself to get the same
information. But it's not worth changing the API and breaking external
modules for that.

- Heikki