Re: [PATCH 2/6] boot: android: Add image_android_get_version()
Hi Igor,
Thank you for the review.
On lun., juin 10, 2024 at 11:20, Igor Opaniuk wrote:
> Hi Mattijs,
>
> On Thu, Jun 6, 2024 at 2:24 PM Mattijs Korpershoek
> wrote:
>>
>> When reading a boot image header, we may need to retrieve the header
>> version.
>>
>> Add a helper function for it.
>>
>> Signed-off-by: Mattijs Korpershoek
>> ---
>> boot/image-android.c | 7 ++-
>> include/image.h | 7 +++
>> 2 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/boot/image-android.c b/boot/image-android.c
>> index ddd8ffd5e540..4f8fb51585eb 100644
>> --- a/boot/image-android.c
>> +++ b/boot/image-android.c
>> @@ -185,7 +185,7 @@ bool android_image_get_data(const void *boot_hdr, const
>> void *vendor_boot_hdr,
>> return false;
>> }
>>
>> - if (((struct andr_boot_img_hdr_v0 *)boot_hdr)->header_version > 2) {
>> + if (android_image_get_version(boot_hdr) > 2) {
>> if (!vendor_boot_hdr) {
>> printf("For boot header v3+ vendor boot image has to
>> be provided\n");
>> return false;
>> @@ -203,6 +203,11 @@ bool android_image_get_data(const void *boot_hdr, const
>> void *vendor_boot_hdr,
>> return true;
>> }
>>
>> +u32 android_image_get_version(const void *hdr)
>> +{
>> + return ((struct andr_boot_img_hdr_v0 *)hdr)->header_version;
>> +}
>> +
>> static ulong android_image_get_kernel_addr(struct andr_image_data *img_data)
>> {
>> /*
>> diff --git a/include/image.h b/include/image.h
>> index acffd17e0dfd..18e5ced5ab42 100644
>> --- a/include/image.h
>> +++ b/include/image.h
>> @@ -1963,6 +1963,13 @@ bool is_android_boot_image_header(const void *hdr);
>> */
>> bool is_android_vendor_boot_image_header(const void *vendor_boot_img);
>>
>> +/**
>> + * android_image_get_version() - Retrieve the boot.img version
>> + *
>> + * Return: Android boot image header version.
>> + */
>> +u32 android_image_get_version(const void *hdr);
>> +
>> /**
>> * get_abootimg_addr() - Get Android boot image address
>> *
>>
>> --
>> 2.45.0
>>
> why introduce a helper function if there is only one user of it?
I added a second user of the helper function in patch 5/6.
>
> android_image_get_data() expects andr_boot_img_hdr_v0 anyway,
> as it has an explicit check for it in the very beginning
> (is_android_boot_image_header()).
Right.
>
> Have you considered adjusting android_image_get_data() declaration, and just
> use
> andr_boot_img_hdr_v0 *boot_hdr as first param instead (like it's done
> for example in
> android_boot_image_v0_v1_v2_parse_hdr()) and then rely on implicit
> cast when this
> function is used.
>
> this is of course all a matter of preference, just thinking out loud
I've given this some more thought, and since I'm already using
struct andr_boot_img_hdr_v0 in bootmeth_android/scan_boot_part(), I will
drop this patch for v2.
The helper seems indeed a bit useless given that we can use the struct's
member for this.
Thanks!
>
> --
> Best regards - Atentamente - Meilleures salutations
>
> Igor Opaniuk
>
> mailto: igor.opan...@gmail.com
> skype: igor.opanyuk
> https://www.linkedin.com/in/iopaniuk
Re: [PATCH 2/6] boot: android: Add image_android_get_version()
Hi Mattijs,
On Thu, Jun 6, 2024 at 2:24 PM Mattijs Korpershoek
wrote:
>
> When reading a boot image header, we may need to retrieve the header
> version.
>
> Add a helper function for it.
>
> Signed-off-by: Mattijs Korpershoek
> ---
> boot/image-android.c | 7 ++-
> include/image.h | 7 +++
> 2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/boot/image-android.c b/boot/image-android.c
> index ddd8ffd5e540..4f8fb51585eb 100644
> --- a/boot/image-android.c
> +++ b/boot/image-android.c
> @@ -185,7 +185,7 @@ bool android_image_get_data(const void *boot_hdr, const
> void *vendor_boot_hdr,
> return false;
> }
>
> - if (((struct andr_boot_img_hdr_v0 *)boot_hdr)->header_version > 2) {
> + if (android_image_get_version(boot_hdr) > 2) {
> if (!vendor_boot_hdr) {
> printf("For boot header v3+ vendor boot image has to
> be provided\n");
> return false;
> @@ -203,6 +203,11 @@ bool android_image_get_data(const void *boot_hdr, const
> void *vendor_boot_hdr,
> return true;
> }
>
> +u32 android_image_get_version(const void *hdr)
> +{
> + return ((struct andr_boot_img_hdr_v0 *)hdr)->header_version;
> +}
> +
> static ulong android_image_get_kernel_addr(struct andr_image_data *img_data)
> {
> /*
> diff --git a/include/image.h b/include/image.h
> index acffd17e0dfd..18e5ced5ab42 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -1963,6 +1963,13 @@ bool is_android_boot_image_header(const void *hdr);
> */
> bool is_android_vendor_boot_image_header(const void *vendor_boot_img);
>
> +/**
> + * android_image_get_version() - Retrieve the boot.img version
> + *
> + * Return: Android boot image header version.
> + */
> +u32 android_image_get_version(const void *hdr);
> +
> /**
> * get_abootimg_addr() - Get Android boot image address
> *
>
> --
> 2.45.0
>
why introduce a helper function if there is only one user of it?
android_image_get_data() expects andr_boot_img_hdr_v0 anyway,
as it has an explicit check for it in the very beginning
(is_android_boot_image_header()).
Have you considered adjusting android_image_get_data() declaration, and just use
andr_boot_img_hdr_v0 *boot_hdr as first param instead (like it's done
for example in
android_boot_image_v0_v1_v2_parse_hdr()) and then rely on implicit
cast when this
function is used.
this is of course all a matter of preference, just thinking out loud
--
Best regards - Atentamente - Meilleures salutations
Igor Opaniuk
mailto: igor.opan...@gmail.com
skype: igor.opanyuk
https://www.linkedin.com/in/iopaniuk
[PATCH 2/6] boot: android: Add image_android_get_version()
When reading a boot image header, we may need to retrieve the header
version.
Add a helper function for it.
Signed-off-by: Mattijs Korpershoek
---
boot/image-android.c | 7 ++-
include/image.h | 7 +++
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/boot/image-android.c b/boot/image-android.c
index ddd8ffd5e540..4f8fb51585eb 100644
--- a/boot/image-android.c
+++ b/boot/image-android.c
@@ -185,7 +185,7 @@ bool android_image_get_data(const void *boot_hdr, const
void *vendor_boot_hdr,
return false;
}
- if (((struct andr_boot_img_hdr_v0 *)boot_hdr)->header_version > 2) {
+ if (android_image_get_version(boot_hdr) > 2) {
if (!vendor_boot_hdr) {
printf("For boot header v3+ vendor boot image has to be
provided\n");
return false;
@@ -203,6 +203,11 @@ bool android_image_get_data(const void *boot_hdr, const
void *vendor_boot_hdr,
return true;
}
+u32 android_image_get_version(const void *hdr)
+{
+ return ((struct andr_boot_img_hdr_v0 *)hdr)->header_version;
+}
+
static ulong android_image_get_kernel_addr(struct andr_image_data *img_data)
{
/*
diff --git a/include/image.h b/include/image.h
index acffd17e0dfd..18e5ced5ab42 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1963,6 +1963,13 @@ bool is_android_boot_image_header(const void *hdr);
*/
bool is_android_vendor_boot_image_header(const void *vendor_boot_img);
+/**
+ * android_image_get_version() - Retrieve the boot.img version
+ *
+ * Return: Android boot image header version.
+ */
+u32 android_image_get_version(const void *hdr);
+
/**
* get_abootimg_addr() - Get Android boot image address
*
--
2.45.0
