[U-Boot] [PATCH 1/1] avb2.0: add get_size_of_partition()

[Andrew F. Davis]

On 07/13/2018 12:09 PM, Igor Opaniuk wrote:
> Hi Andrew,
> Sorry I missed your message.
> 
> On 9 July 2018 at 18:21, Andrew F. Davis <afd at ti.com> wrote:
>> On 07/09/2018 09:52 AM, Sam Protsenko wrote:
>>> On Mon, Jul 9, 2018 at 3:15 PM, Igor Opaniuk <igor.opaniuk at linaro.org> wrote:
>>>> Implement get_size_of_partition() operation,
>>>> which is required by the latest upstream libavb [1].
>>>>
>>>> [1] https://android.googlesource.com/platform/external/avb/+/master/README.md
>>>>
>>
>>
>> I may have missed it, where in here do we need this information? I looks
>> to be passed in on the command line for most ops. Has a new function
>> been added?
> 
> right, it was introduced in 417e8133af46 ("libavb: Load entire
> partition if |allow_verification_error| is true.").
> When I included the latest libavb for the avb v2 patch series (in v1
> the was out-dated version, AFAIK ~1y old),
> I noticed this new Avb operation, although it didn't have any impact
> on avb verify functionality.
> (I was receiving just a warning that no get_size_of_partition is set
> in AvbOps structure)
> 
> So I decided to introduce the implementation of this function in a
> separate patch.
> 
>>
>>>> Signed-off-by: Igor Opaniuk <igor.opaniuk at linaro.org>
>>>> ---
>>>
>>> Reviewed-by: Sam Protsenko <semen.protsenko at linaro.org>
>>>
>>>>  common/avb_verify.c | 33 ++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 32 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/common/avb_verify.c b/common/avb_verify.c
>>>> index f9a00f8..5eabab0 100644
>>>> --- a/common/avb_verify.c
>>>> +++ b/common/avb_verify.c
>>>> @@ -699,6 +699,37 @@ static AvbIOResult get_unique_guid_for_partition(AvbOps *ops,
>>>>  }
>>>>
>>>>  /**
>>>> + * get_size_of_partition() - gets the size of a partition identified
>>>> + * by a string name
>>>> + *
>>>> + * @ops: contains AVB ops handlers
>>>> + * @partition: partition name (NUL-terminated UTF-8 string)
>>>> + * @out_size_num_bytes: returns the value of a partition size
>>>> + *
>>>> + * @return:
>>>> + *      AVB_IO_RESULT_OK, on success (GUID found)
>>>> + *      AVB_IO_RESULT_ERROR_IO, out_size_num_bytes is NULL
>>
>>
>> This does not seems to be the right error code for this, this implies a
>> hardware error, maybe AVB_IO_RESULT_ERROR_INSUFFICIENT_SPACE is a better
>> choice? 'out_size_num_bytes' is a buffer in a way (to 8 bytes).
> 
> Frankly, I chose the most "abstract" (if I can say that) error code,
> as there is no any in AVB that resembles POSIX EINVAL.


But it is not abstract or generic, it is very specific:

"is returned if the underlying hardware encountered an I/O error."

If no I/O error has occurred, you should not be returning this error code.


> Regarding AVB_IO_RESULT_ERROR_INSUFFICIENT_SPACE, which
> "is returned if a buffer is too small for the requested operation",
> I'm not sure that it's proper error either.
> 


Neither are perfectly proper, but lets not make the user think their HW
is broken.


> There is a question spinning in my mind "should we do any param verification?",
> as only libavb is using these functions,
> and no theoretical case is possible when an invalid value is provided.
> 


Sanity checks never hurt, trust nobody, not even yourself.

For parameters input by user then of course verify. But parameters only
populated by other code, then it depends.

It matters what will happen when at some point when someone will mess up
the code and this function will get called with bad params. In this
particular case all that will happen is a null pointer exception will
get thrown, the dev will see it instantly and fix the code. So it's not
needed here.

The ones that really need it are ones that the bad parameter will
propagate silently and break somewhere else much later and may even make
it back in to production code bases if the problem it causes isn't
immediately obvious enough.

Andrew


>>
>> Andrew
>>
>>
>>>> + *      AVB_IO_RESULT_ERROR_NO_SUCH_PARTITION, if partition was not found
>>>> + */
>>>> +static AvbIOResult get_size_of_partition(AvbOps *ops,
>>>> +                                        const char *partition,
>>>> +                                        u64 *out_size_num_bytes)
>>>> +{
>>>> +       struct mmc_part *part;
>>>> +
>>>> +       if (!out_size_num_bytes)
>>>> +               return AVB_IO_RESULT_ERROR_IO;
>>>> +
>>>> +       part = get_partition(ops, partition);
>>>> +       if (!part)
>>>> +               return AVB_IO_RESULT_ERROR_NO_SUCH_PARTITION;
>>>> +
>>>> +       *out_size_num_bytes = part->info.blksz * part->info.size;
>>>> +
>>>> +       return AVB_IO_RESULT_OK;
>>>> +}
>>>> +
>>>> +/**
>>>>   * ============================================================================
>>>>   * AVB2.0 AvbOps alloc/initialisation/free
>>>>   * ============================================================================
>>>> @@ -721,7 +752,7 @@ AvbOps *avb_ops_alloc(int boot_device)
>>>>         ops_data->ops.read_is_device_unlocked = read_is_device_unlocked;
>>>>         ops_data->ops.get_unique_guid_for_partition =
>>>>                 get_unique_guid_for_partition;
>>>> -
>>>> +       ops_data->ops.get_size_of_partition = get_size_of_partition;
>>>>         ops_data->mmc_dev = boot_device;
>>>>
>>>>         return &ops_data->ops;
>>>> --
>>>> 2.7.4
>>>>
> 
> 
>