| Message ID | 1531138554-7429-1-git-send-email-igor.opaniuk@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/1] avb2.0: add get_size_of_partition() | expand |
On Mon, Jul 9, 2018 at 3:15 PM, Igor Opaniuk <igor.opaniuk@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 > > Signed-off-by: Igor Opaniuk <igor.opaniuk@linaro.org> > --- Reviewed-by: Sam Protsenko <semen.protsenko@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 > + * 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 >
On 07/09/2018 09:52 AM, Sam Protsenko wrote: > On Mon, Jul 9, 2018 at 3:15 PM, Igor Opaniuk <igor.opaniuk@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? >> Signed-off-by: Igor Opaniuk <igor.opaniuk@linaro.org> >> --- > > Reviewed-by: Sam Protsenko <semen.protsenko@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).. 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 >>
On Mon, Jul 09, 2018 at 03:15:54PM +0300, Igor Opaniuk 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 > > Signed-off-by: Igor Opaniuk <igor.opaniuk@linaro.org> > --- > common/avb_verify.c | 33 ++++++++++++++++++++++++++++++++- > 1 file changed, 32 insertions(+), 1 deletion(-) Hi Igor, Is there a way to play with and smoke-test libavb on sandbox? FWIW currently, independently on this patch, menuconfig interface allows me to select CONFIG_LIBAVB, but then U-Boot compilation fails as below: $ make defconfig $ make menuconfig => select LIBAVB=y $ make ---8<--- In file included from common/avb_verify.c:7:0: include/avb_verify.h: In function ‘get_sector_buf_size’: include/avb_verify.h:70:17: error: ‘CONFIG_FASTBOOT_BUF_SIZE’ undeclared (first use in this function); did you mean ‘CONFIG_PRE_CON_BUF_SZ’? return (size_t)CONFIG_FASTBOOT_BUF_SIZE; ^~~~~~~~~~~~~~~~~~~~~~~~ CONFIG_PRE_CON_BUF_SZ include/avb_verify.h:70:17: note: each undeclared identifier is reported only once for each function it appears in include/avb_verify.h: In function ‘get_sector_buf’: include/avb_verify.h:75:17: error: ‘CONFIG_FASTBOOT_BUF_ADDR’ undeclared (first use in this function); did you mean ‘CONFIG_PRE_CON_BUF_ADDR’? return (void *)CONFIG_FASTBOOT_BUF_ADDR; ^~~~~~~~~~~~~~~~~~~~~~~~ CONFIG_PRE_CON_BUF_ADDR CC env/attr.o ---8<--- If there were an easy way to smoke-test libavb on sandbox, I think people like me would be more motivated to provide their Tested-by in addition to their review (just my feeling). Best regards, Eugeniu.
Hi Eugeniu, Thanks for reporting this issue, LIBAVB should depend on CONFIG_FASTBOOT, as fastboot buffer is re-used (which initially is used in the fastboot protocol for downloads) for mmc read/write AvbOps (and buffer size is configured by setting CONFIG_FASTBOOT_BUF_ADDR and CONFIG_FASTBOOT_BUF_SIZE) The problem is that both CONFIG_FASTBOOT_BUF_ADDR and CONFIG_FASTBOOT_BUF_SIZE are defined for most platforms, and this is how I missed this issue. Will fix today and re-test, Thanks On 9 July 2018 at 18:33, Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > On Mon, Jul 09, 2018 at 03:15:54PM +0300, Igor Opaniuk 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 >> >> Signed-off-by: Igor Opaniuk <igor.opaniuk@linaro.org> >> --- >> common/avb_verify.c | 33 ++++++++++++++++++++++++++++++++- >> 1 file changed, 32 insertions(+), 1 deletion(-) > > > Hi Igor, > > Is there a way to play with and smoke-test libavb on sandbox? > FWIW currently, independently on this patch, menuconfig interface allows me > to select CONFIG_LIBAVB, but then U-Boot compilation fails as below: > > $ make defconfig > $ make menuconfig => select LIBAVB=y > $ make > > ---8<--- > In file included from common/avb_verify.c:7:0: > include/avb_verify.h: In function ‘get_sector_buf_size’: > include/avb_verify.h:70:17: error: ‘CONFIG_FASTBOOT_BUF_SIZE’ undeclared (first use in this function); did you mean ‘CONFIG_PRE_CON_BUF_SZ’? > return (size_t)CONFIG_FASTBOOT_BUF_SIZE; > ^~~~~~~~~~~~~~~~~~~~~~~~ > CONFIG_PRE_CON_BUF_SZ > include/avb_verify.h:70:17: note: each undeclared identifier is reported only once for each function it appears in > include/avb_verify.h: In function ‘get_sector_buf’: > include/avb_verify.h:75:17: error: ‘CONFIG_FASTBOOT_BUF_ADDR’ undeclared (first use in this function); did you mean ‘CONFIG_PRE_CON_BUF_ADDR’? > return (void *)CONFIG_FASTBOOT_BUF_ADDR; > ^~~~~~~~~~~~~~~~~~~~~~~~ > CONFIG_PRE_CON_BUF_ADDR > CC env/attr.o > ---8<--- > > If there were an easy way to smoke-test libavb on sandbox, I think > people like me would be more motivated to provide their Tested-by > in addition to their review (just my feeling). > > Best regards, > Eugeniu.
Hi Andrew, Sorry I missed your message. On 9 July 2018 at 18:21, Andrew F. Davis <afd@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@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@linaro.org> >>> --- >> >> Reviewed-by: Sam Protsenko <semen.protsenko@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. 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. 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. > > 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 >>>
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@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@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@linaro.org> >>>> --- >>> >>> Reviewed-by: Sam Protsenko <semen.protsenko@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 >>>> > > >
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 + * 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;
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 Signed-off-by: Igor Opaniuk <igor.opaniuk@linaro.org> --- common/avb_verify.c | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-)