Re: [PATCH v1 2/5] fastboot: implement "getvar all"

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

Hi Svyatoslav,

Thank you for your patch.

On mar., nov. 07, 2023 at 14:42, Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> From: Ion Agorria <ion@agorria.com>
>
> This commit implements "fastboot getvar all" listing
> by iterating the existing dispatchers that don't require
> parameters (as we pass NULL), uses fastboot multiresponse.
>
> Signed-off-by: Ion Agorria <ion@agorria.com>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>

Some small comments below.

With those addressed, please add:

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>  doc/android/fastboot-protocol.rst |  3 ++
>  drivers/fastboot/fb_command.c     |  3 ++
>  drivers/fastboot/fb_getvar.c      | 75 +++++++++++++++++++++++++------
>  include/fastboot-internal.h       |  7 +++
>  4 files changed, 75 insertions(+), 13 deletions(-)
>
> diff --git a/doc/android/fastboot-protocol.rst b/doc/android/fastboot-protocol.rst
> index e8cbd7f24e..8bd6d7168f 100644
> --- a/doc/android/fastboot-protocol.rst
> +++ b/doc/android/fastboot-protocol.rst
> @@ -173,6 +173,9 @@ The various currently defined names are::
>                        bootloader requiring a signature before
>                        it will install or boot images.
>  
> +  all                 Provides all info from commands above as
> +                      they were called one by one
> +
>  Names starting with a lowercase character are reserved by this
>  specification.  OEM-specific names should not start with lowercase
>  characters.
> diff --git a/drivers/fastboot/fb_command.c b/drivers/fastboot/fb_command.c
> index ab72d8c781..6f621df074 100644
> --- a/drivers/fastboot/fb_command.c
> +++ b/drivers/fastboot/fb_command.c
> @@ -156,6 +156,9 @@ int fastboot_handle_command(char *cmd_string, char *response)
>  void fastboot_multiresponse(int cmd, char *response)
>  {
>  	switch (cmd) {
> +	case FASTBOOT_COMMAND_GETVAR:
> +		fastboot_getvar_all(response);
> +		break;
>  	default:
>  		fastboot_fail("Unknown multiresponse command", response);
>  		break;
> diff --git a/drivers/fastboot/fb_getvar.c b/drivers/fastboot/fb_getvar.c
> index 8cb8ffa2c6..fc5e1dac87 100644
> --- a/drivers/fastboot/fb_getvar.c
> +++ b/drivers/fastboot/fb_getvar.c
> @@ -29,53 +29,67 @@ static void getvar_is_userspace(char *var_parameter, char *response);
>  
>  static const struct {
>  	const char *variable;
> +	bool list;
>  	void (*dispatch)(char *var_parameter, char *response);
>  } getvar_dispatch[] = {
>  	{
>  		.variable = "version",
> -		.dispatch = getvar_version
> +		.dispatch = getvar_version,
> +		.list = true,
>  	}, {
>  		.variable = "version-bootloader",
> -		.dispatch = getvar_version_bootloader
> +		.dispatch = getvar_version_bootloader,
> +		.list = true
>  	}, {
>  		.variable = "downloadsize",
> -		.dispatch = getvar_downloadsize
> +		.dispatch = getvar_downloadsize,
> +		.list = true
>  	}, {
>  		.variable = "max-download-size",
> -		.dispatch = getvar_downloadsize
> +		.dispatch = getvar_downloadsize,
> +		.list = true
>  	}, {
>  		.variable = "serialno",
> -		.dispatch = getvar_serialno
> +		.dispatch = getvar_serialno,
> +		.list = true
>  	}, {
>  		.variable = "version-baseband",
> -		.dispatch = getvar_version_baseband
> +		.dispatch = getvar_version_baseband,
> +		.list = true
>  	}, {
>  		.variable = "product",
> -		.dispatch = getvar_product
> +		.dispatch = getvar_product,
> +		.list = true
>  	}, {
>  		.variable = "platform",
> -		.dispatch = getvar_platform
> +		.dispatch = getvar_platform,
> +		.list = true
>  	}, {
>  		.variable = "current-slot",
> -		.dispatch = getvar_current_slot
> +		.dispatch = getvar_current_slot,
> +		.list = true
>  #if IS_ENABLED(CONFIG_FASTBOOT_FLASH)
>  	}, {
>  		.variable = "has-slot",
> -		.dispatch = getvar_has_slot
> +		.dispatch = getvar_has_slot,
> +		.list = false
>  #endif
>  #if IS_ENABLED(CONFIG_FASTBOOT_FLASH_MMC)
>  	}, {
>  		.variable = "partition-type",
> -		.dispatch = getvar_partition_type
> +		.dispatch = getvar_partition_type,
> +		.list = false
>  #endif
>  #if IS_ENABLED(CONFIG_FASTBOOT_FLASH)
>  	}, {
>  		.variable = "partition-size",
> -		.dispatch = getvar_partition_size
> +		.dispatch = getvar_partition_size,
> +		.list = false
>  #endif
>  	}, {
>  		.variable = "is-userspace",
> -		.dispatch = getvar_is_userspace
> +		.dispatch = getvar_is_userspace,
> +		.list = true
>  	}
>  };
>  
> @@ -237,6 +251,38 @@ static void getvar_is_userspace(char *var_parameter, char *response)
>  	fastboot_okay("no", response);
>  }
>  
> +int current_all_dispatch;

static please, as this is only used in drivers/fastboot/fb_getvar.c

> +void fastboot_getvar_all(char *response)
> +{
> +	/*
> +	 * Find a dispatch getvar that can be listed and send
> +	 * it as INFO until we reach the end.
> +	 */
> +	while (current_all_dispatch < ARRAY_SIZE(getvar_dispatch)) {
> +		if (!getvar_dispatch[current_all_dispatch].list) {
> +			current_all_dispatch++;
> +			continue;
> +		}
> +
> +		char envstr[FASTBOOT_RESPONSE_LEN] = { 0 };
> +		getvar_dispatch[current_all_dispatch].dispatch(NULL, envstr);

./scripts/checkpatch.pl --strict --u-boot complains a missing line here

> +
> +		char *envstr_start = envstr;

./scripts/checkpatch.pl --strict --u-boot complains a missing line here

> +		if (!strncmp("OKAY", envstr, 4) || !strncmp("FAIL", envstr, 4))
> +			envstr_start += 4;
> +
> +		fastboot_response("INFO", response, "%s: %s",
> +				  getvar_dispatch[current_all_dispatch].variable,
> +				  envstr_start);
> +
> +		current_all_dispatch++;
> +		return;
> +	}
> +
> +	fastboot_response("OKAY", response, NULL);
> +	current_all_dispatch = 0;
> +}
> +
>  /**
>   * fastboot_getvar() - Writes variable indicated by cmd_parameter to response.
>   *
> @@ -254,6 +300,9 @@ void fastboot_getvar(char *cmd_parameter, char *response)
>  {
>  	if (!cmd_parameter) {
>  		fastboot_fail("missing var", response);
> +	} else if (!strncmp("all", cmd_parameter, 3) && strlen(cmd_parameter) == 3) {
> +		current_all_dispatch = 0;
> +		fastboot_response("MORE", response, NULL);
>  	} else {
>  #define FASTBOOT_ENV_PREFIX	"fastboot."
>  		int i;
> diff --git a/include/fastboot-internal.h b/include/fastboot-internal.h
> index bf2f2b3c89..610d4f9141 100644
> --- a/include/fastboot-internal.h
> +++ b/include/fastboot-internal.h
> @@ -18,6 +18,13 @@ extern u32 fastboot_buf_size;
>   */
>  extern void (*fastboot_progress_callback)(const char *msg);
>  
> +/**
> + * fastboot_getvar_all() - Writes current variable being listed from "all" to response.
> + *
> + * @response: Pointer to fastboot response buffer
> + */
> +void fastboot_getvar_all(char *response);
> +
>  /**
>   * fastboot_getvar() - Writes variable indicated by cmd_parameter to response.
>   *
> -- 
> 2.40.1