[2/6] efi_loader: add efi_dp_from_name()

Factor out efi_set_bootdev() and extract efi_dp_from_name().
This function will be used to set a boot device in efishell command.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/bootefi.c                    | 42 ++++++----------------------
 include/efi_loader.h             |  4 +++
 lib/efi_loader/efi_device_path.c | 47 ++++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 34 deletions(-)

On 10/17/2018 09:32 AM, AKASHI Takahiro wrote:
> Factor out efi_set_bootdev() and extract efi_dp_from_name().
> This function will be used to set a boot device in efishell command.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  cmd/bootefi.c                    | 42 ++++++----------------------
>  include/efi_loader.h             |  4 +++
>  lib/efi_loader/efi_device_path.c | 47 ++++++++++++++++++++++++++++++++
>  3 files changed, 59 insertions(+), 34 deletions(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 99f5b2b95706..b3e2845120fe 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -647,45 +647,19 @@ U_BOOT_CMD(
>  
>  void efi_set_bootdev(const char *dev, const char *devnr, const char *path)
>  {
> -	char filename[32] = { 0 }; /* dp->str is u16[32] long */
> -	char *s;
> +	struct efi_device_path *device, *image;
> +	efi_status_t ret;
>  
>  	/* efi_set_bootdev is typically called repeatedly, recover memory */
>  	efi_free_pool(bootefi_device_path);
>  	efi_free_pool(bootefi_image_path);
> -	/* If blk_get_device_part_str fails, avoid duplicate free. */
> -	bootefi_device_path = NULL;
> -	bootefi_image_path = NULL;
> -
> -	if (strcmp(dev, "Net")) {
> -		struct blk_desc *desc;
> -		disk_partition_t fs_partition;
> -		int part;
> -
> -		part = blk_get_device_part_str(dev, devnr, &desc, &fs_partition,
> -					       1);
> -		if (part < 0)
> -			return;
> -
> -		bootefi_device_path = efi_dp_from_part(desc, part);
> -	} else {
> -#ifdef CONFIG_NET
> -		bootefi_device_path = efi_dp_from_eth();
> -#endif
> -	}
> -
> -	if (!path)
> -		return;
>  
> -	if (strcmp(dev, "Net")) {
> -		/* Add leading / to fs paths, because they're absolute */
> -		snprintf(filename, sizeof(filename), "/%s", path);
> +	ret = efi_dp_from_name(dev, devnr, path, &device, &image);
> +	if (ret == EFI_SUCCESS) {
> +		bootefi_device_path = device;
> +		bootefi_image_path = image;
>  	} else {
> -		snprintf(filename, sizeof(filename), "%s", path);
> +		bootefi_device_path = NULL;
> +		bootefi_image_path = NULL;
>  	}
> -	/* DOS style file path: */
> -	s = filename;
> -	while ((s = strchr(s, '/')))
> -		*s++ = '\\';
> -	bootefi_image_path = efi_dp_from_file(NULL, 0, filename);
>  }
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 74df070316a0..b73fbb6de23f 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -425,6 +425,10 @@ const struct efi_device_path *efi_dp_last_node(
>  efi_status_t efi_dp_split_file_path(struct efi_device_path *full_path,
>  				    struct efi_device_path **device_path,
>  				    struct efi_device_path **file_path);
> +efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
> +			      const char *path,
> +			      struct efi_device_path **device,
> +			      struct efi_device_path **file);
>  
>  #define EFI_DP_TYPE(_dp, _type, _subtype) \
>  	(((_dp)->type == DEVICE_PATH_TYPE_##_type) && \
> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> index 5a61a1c1dcf9..3af444147283 100644
> --- a/lib/efi_loader/efi_device_path.c
> +++ b/lib/efi_loader/efi_device_path.c
> @@ -942,3 +942,50 @@ efi_status_t efi_dp_split_file_path(struct efi_device_path *full_path,
>  	*file_path = fp;
>  	return EFI_SUCCESS;
>  }
> +
> +efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
> +			      const char *path,
> +			      struct efi_device_path **device,
> +			      struct efi_device_path **file)
> +{
> +	int is_net;
> +	struct blk_desc *desc = NULL;
> +	disk_partition_t fs_partition;
> +	int part = 0;
> +	char filename[32] = { 0 }; /* dp->str is u16[32] long */
> +	char *s;
> +
> +	if (!device || (path && !file))
> +		return EFI_INVALID_PARAMETER;
> +
> +	is_net = !strcmp(dev, "Net");
> +	if (!is_net) {
> +		part = blk_get_device_part_str(dev, devnr, &desc, &fs_partition,
> +					       1);
> +		if (part < 0)
> +			return EFI_INVALID_PARAMETER;
> +
> +		*device = efi_dp_from_part(desc, part);
> +	} else {
> +#ifdef CONFIG_NET
> +		*device = efi_dp_from_eth();
> +#endif
> +	}
> +
> +	if (!path)
> +		return EFI_SUCCESS;
> +
> +	if (!is_net) {
> +		/* Add leading / to fs paths, because they're absolute */
> +		snprintf(filename, sizeof(filename), "/%s", path);

This coding creates undefined behavior. You have to use two different
variables.

> +	} else {
> +		snprintf(filename, sizeof(filename), "%s", path);

Same here.

@Alex:
Please, remove the patch from efi-next until it is fixed.

Best regards

Heinrich

> +	}
> +	/* DOS style file path: */
> +	s = filename;
> +	while ((s = strchr(s, '/')))
> +		*s++ = '\\';
> +	*file = efi_dp_from_file(NULL, 0, filename);
> +
> +	return EFI_SUCCESS;
> +}
>

On 10/17/2018 12:46 PM, Heinrich Schuchardt wrote:
> On 10/17/2018 09:32 AM, AKASHI Takahiro wrote:
>> Factor out efi_set_bootdev() and extract efi_dp_from_name().
>> This function will be used to set a boot device in efishell command.
>>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> ---
>>  cmd/bootefi.c                    | 42 ++++++----------------------
>>  include/efi_loader.h             |  4 +++
>>  lib/efi_loader/efi_device_path.c | 47 ++++++++++++++++++++++++++++++++
>>  3 files changed, 59 insertions(+), 34 deletions(-)
>>
>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>> index 99f5b2b95706..b3e2845120fe 100644
>> --- a/cmd/bootefi.c
>> +++ b/cmd/bootefi.c
>> @@ -647,45 +647,19 @@ U_BOOT_CMD(
>>  
>>  void efi_set_bootdev(const char *dev, const char *devnr, const char *path)
>>  {
>> -	char filename[32] = { 0 }; /* dp->str is u16[32] long */
>> -	char *s;
>> +	struct efi_device_path *device, *image;
>> +	efi_status_t ret;
>>  
>>  	/* efi_set_bootdev is typically called repeatedly, recover memory */
>>  	efi_free_pool(bootefi_device_path);
>>  	efi_free_pool(bootefi_image_path);
>> -	/* If blk_get_device_part_str fails, avoid duplicate free. */
>> -	bootefi_device_path = NULL;
>> -	bootefi_image_path = NULL;
>> -
>> -	if (strcmp(dev, "Net")) {
>> -		struct blk_desc *desc;
>> -		disk_partition_t fs_partition;
>> -		int part;
>> -
>> -		part = blk_get_device_part_str(dev, devnr, &desc, &fs_partition,
>> -					       1);
>> -		if (part < 0)
>> -			return;
>> -
>> -		bootefi_device_path = efi_dp_from_part(desc, part);
>> -	} else {
>> -#ifdef CONFIG_NET
>> -		bootefi_device_path = efi_dp_from_eth();
>> -#endif
>> -	}
>> -
>> -	if (!path)
>> -		return;
>>  
>> -	if (strcmp(dev, "Net")) {
>> -		/* Add leading / to fs paths, because they're absolute */
>> -		snprintf(filename, sizeof(filename), "/%s", path);
>> +	ret = efi_dp_from_name(dev, devnr, path, &device, &image);
>> +	if (ret == EFI_SUCCESS) {
>> +		bootefi_device_path = device;
>> +		bootefi_image_path = image;
>>  	} else {
>> -		snprintf(filename, sizeof(filename), "%s", path);
>> +		bootefi_device_path = NULL;
>> +		bootefi_image_path = NULL;
>>  	}
>> -	/* DOS style file path: */
>> -	s = filename;
>> -	while ((s = strchr(s, '/')))
>> -		*s++ = '\\';
>> -	bootefi_image_path = efi_dp_from_file(NULL, 0, filename);
>>  }
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 74df070316a0..b73fbb6de23f 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -425,6 +425,10 @@ const struct efi_device_path *efi_dp_last_node(
>>  efi_status_t efi_dp_split_file_path(struct efi_device_path *full_path,
>>  				    struct efi_device_path **device_path,
>>  				    struct efi_device_path **file_path);
>> +efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
>> +			      const char *path,
>> +			      struct efi_device_path **device,
>> +			      struct efi_device_path **file);
>>  
>>  #define EFI_DP_TYPE(_dp, _type, _subtype) \
>>  	(((_dp)->type == DEVICE_PATH_TYPE_##_type) && \
>> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
>> index 5a61a1c1dcf9..3af444147283 100644
>> --- a/lib/efi_loader/efi_device_path.c
>> +++ b/lib/efi_loader/efi_device_path.c
>> @@ -942,3 +942,50 @@ efi_status_t efi_dp_split_file_path(struct efi_device_path *full_path,
>>  	*file_path = fp;
>>  	return EFI_SUCCESS;
>>  }
>> +
>> +efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
>> +			      const char *path,
>> +			      struct efi_device_path **device,
>> +			      struct efi_device_path **file)
>> +{
>> +	int is_net;
>> +	struct blk_desc *desc = NULL;
>> +	disk_partition_t fs_partition;
>> +	int part = 0;
>> +	char filename[32] = { 0 }; /* dp->str is u16[32] long */
>> +	char *s;
>> +
>> +	if (!device || (path && !file))
>> +		return EFI_INVALID_PARAMETER;
>> +
>> +	is_net = !strcmp(dev, "Net");
>> +	if (!is_net) {
>> +		part = blk_get_device_part_str(dev, devnr, &desc, &fs_partition,
>> +					       1);
>> +		if (part < 0)
>> +			return EFI_INVALID_PARAMETER;
>> +
>> +		*device = efi_dp_from_part(desc, part);
>> +	} else {
>> +#ifdef CONFIG_NET
>> +		*device = efi_dp_from_eth();
>> +#endif
>> +	}
>> +
>> +	if (!path)
>> +		return EFI_SUCCESS;
>> +
>> +	if (!is_net) {
>> +		/* Add leading / to fs paths, because they're absolute */
>> +		snprintf(filename, sizeof(filename), "/%s", path);
> 
> This coding creates undefined behavior. You have to use two different
> variables.

Sorry I must be blind.

Heinrich
> 
>> +	} else {
>> +		snprintf(filename, sizeof(filename), "%s", path);
> 
> Same here.
> 
> @Alex:
> Please, remove the patch from efi-next until it is fixed.
> 
> Best regards
> 
> Heinrich
> 
>> +	}
>> +	/* DOS style file path: */
>> +	s = filename;
>> +	while ((s = strchr(s, '/')))
>> +		*s++ = '\\';
>> +	*file = efi_dp_from_file(NULL, 0, filename);
>> +
>> +	return EFI_SUCCESS;
>> +}
>>
> 
>