Re: [PATCH] grub-mkpasswd-pbkdf2: Simplify the main function implementation

[Tianjia Zhang <tianjia.zhang@linux.alibaba.com>]

On 3/15/21 9:26 AM, Glenn Washburn wrote:
> On Sun, 14 Mar 2021 19:09:36 +0800
> Tianjia Zhang <tianjia.zhang@linux.alibaba.com> wrote:
> 
>> ping.
>>
>> Thanks,
>> Tianjia
>>
>> On 11/18/20 2:58 PM, Tianjia Zhang wrote:
>>> Allocate memory if needed, while saving the corresponding release
>>> operation, reducing the amount of code and code complexity.
>>>
>>> Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
>>> ---
>>>    util/grub-mkpasswd-pbkdf2.c | 20 +++++---------------
>>>    1 file changed, 5 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/util/grub-mkpasswd-pbkdf2.c
>>> b/util/grub-mkpasswd-pbkdf2.c index 5805f3c10..cd7943df6 100644
>>> --- a/util/grub-mkpasswd-pbkdf2.c
>>> +++ b/util/grub-mkpasswd-pbkdf2.c
>>> @@ -132,35 +132,25 @@ main (int argc, char *argv[])
>>>          fprintf (stderr, "%s", _("Error in parsing command line
>>> arguments\n")); exit(1);
>>>        }
>>> -
>>> -  buf = xmalloc (arguments.buflen);
>>> -  salt = xmalloc (arguments.saltlen);
>>>      
>>>      printf ("%s", _("Enter password: "));
>>>      if (!grub_password_get (pass1, GRUB_AUTH_MAX_PASSLEN))
>>> -    {
>>> -      free (buf);
>>> -      free (salt);
>>> -      grub_util_error ("%s", _("failure to read password"));
>>> -    }
>>> +    grub_util_error ("%s", _("failure to read password"));
>>>      printf ("%s", _("Reenter password: "));
>>>      if (!grub_password_get (pass2, GRUB_AUTH_MAX_PASSLEN))
>>> -    {
>>> -      free (buf);
>>> -      free (salt);
>>> -      grub_util_error ("%s", _("failure to read password"));
>>> -    }
>>> +    grub_util_error ("%s", _("failure to read password"));
>>>    
>>>      if (strcmp (pass1, pass2) != 0)
>>>        {
>>>          memset (pass1, 0, sizeof (pass1));
>>>          memset (pass2, 0, sizeof (pass2));
>>> -      free (buf);
>>> -      free (salt);
>>>          grub_util_error ("%s", _("passwords don't match"));
>>>        }
>>>      memset (pass2, 0, sizeof (pass2));
>>>    
>>> +  buf = xmalloc (arguments.buflen);
>>> +  salt = xmalloc (arguments.saltlen);
>>> +
>>>      if (grub_get_random (salt, arguments.saltlen))
>>>        {
>>>          memset (pass1, 0, sizeof (pass1));
>>>
> 
> LGTM. I wonder if there's some conventions as with local variable
> declaration, where we try to have memory allocation near the beginning
> of the function as a kind of "declaration" that this should be freed
> before leaving the function.
> 
>    Reviewed-by: Glenn Washburn <development@efficientek.com>
> 
> Glenn
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 


Thanks for your review.

Best regards,
Tianjia