On 26.2.2015 17:01, Martin Basti wrote:
> On 26/02/15 13:06, Petr Spacek wrote:
>> Hello Martin,
>>
>> thank you for patch! This NACK is only aesthetic :-)
>>
>> On 25.2.2015 14:21, Martin Basti wrote:
>>>       if (!check_return_value(rv, "import_wrapped_key: key unwrapping")) {
>>> +        error = 1;
>>> +        goto final;
>>> +    }
>>
>> This exact sequence is repeated many times in the code.
>>
>> I would prefer a C macro like this:
>> #define GOTO_FAIL                        \
>>          do {                            \
>>                  error = 1;                    \
>>                  goto final;                    \
>>          } while(0)
>>
>> This allows more dense code like:
>> if (!test)
>>     GOTO_FAIL;
>>
>> and does not have the risk of missing error = 1 somewhere.
>>
>>> +final:
>>>       if (pkey != NULL)
>>>           EVP_PKEY_free(pkey);
>>> +    if (label != NULL) PyMem_Free(label);
>>> +    if (error){
>>> +        return NULL;
>>> +    }
>>>       return ret;
>>>   }
>> Apparently, this is inconsistent with itself.
>>
>> Please pick one style and use it, e.g.
>> if (label != NULL)
>>     PyMem_Free(label)
>>
>> ... and do not add curly braces when unnecessary.
>>
>> If you want, we can try running $ indent on current sources and committing
>> changes separately so you do not have to make changes like this by hand.
>>
> Thanks. Updated patch attached.

ACK, it works for me.

-- 
Petr^2 Spacek

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to