diff mbox series

[U-Boot,v3,2/3] fastboot: getvar: Refactor fastboot_*_get_part_info() usage

Message ID 20190613181109.6917-3-semen.protsenko@linaro.org
State Accepted
Commit f23a87d5811e38ec88627e0cbace665c22fb7025
Delegated to: Lukasz Majewski
Headers show
Series fastboot: Fix getvar "has-slot" and cleanup | expand

Commit Message

Sam Protsenko June 13, 2019, 6:11 p.m. UTC 
Extract fastboot_*_get_part_info() usage for MMC and NAND into
getvar_get_part_info() function, as it will be needed further in other
functions. This way we can avoid code duplication and mess with
preprocessor directives across all points of usage.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
Reviewed-by: Lukasz Majewski <lukma@denx.de>
Reviewed-by: Igor Opaniuk <igor.opaniuk@toradex.com>
---
Changes in v3:
  - improve comments
  - fill response with fail string for unknown storage types
Changes in v2:
  - add this patch to series

 drivers/fastboot/fb_getvar.c | 58 ++++++++++++++++++++++++++----------
 1 file changed, 42 insertions(+), 16 deletions(-)

Comments

Eugeniu Rosca June 14, 2019, 8:41 a.m. UTC | #1 
On Thu, Jun 13, 2019 at 09:11:08PM +0300, Sam Protsenko wrote:
[..]
> + * Get partition number and size for any storage type.
[..]
> + * @return Partition number or negative value on error
[..]

I think the word 'number' should be blacklisted in the vocabulary
of software development. It can be aliased with 'count' or 'index'
depending on context.

[1] https://marc.info/?l=linux-kernel&m=132388598809954&w=2
[2] https://patchwork.ozlabs.org/patch/1044151/#2108815
Sam Protsenko June 14, 2019, 12:41 p.m. UTC | #2 
Hi Eugeniu,

On Fri, Jun 14, 2019 at 11:42 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> On Thu, Jun 13, 2019 at 09:11:08PM +0300, Sam Protsenko wrote:
> [..]
> > + * Get partition number and size for any storage type.
> [..]
> > + * @return Partition number or negative value on error
> [..]
>
> I think the word 'number' should be blacklisted in the vocabulary
> of software development. It can be aliased with 'count' or 'index'
> depending on context.
>

Although I understand the possible confusion between "index"/"count"
meaning for "number" term, in this particular case I don't agree that
this should be changed to "index" (in this patch at least). Yes, we
shouldn't abuse this term (as any other term, for that matter), but I
guess "partition number" is established expression, which means
"partition index", and everybody understands that:

    $ grep -Ir 'partition number' | wc -l
    46

    $ grep -Ir 'partition index' | wc -l
    2

Similar result can be achieved when grepping in Linux kernel source
tree. Also you can see this expression used in other places, like:

    $ man fdisk | grep 'partition number'
       The partition is a device name followed by a partition number.

All that said, I agree with comment on [1], where "number" can be
really confusing.

So I hope you don't mind if we leave this as is in this patch?

Thanks!

> [1] https://marc.info/?l=linux-kernel&m=132388598809954&w=2
> [2] https://patchwork.ozlabs.org/patch/1044151/#2108815
>
> --
> Best Regards,
> Eugeniu.
Eugeniu Rosca June 14, 2019, 2:30 p.m. UTC | #3 
Hi Sam,

On Fri, Jun 14, 2019 at 03:41:17PM +0300, Sam Protsenko wrote:
[..]
> So I hope you don't mind if we leave this as is in this patch?

Fine. Thanks for the background.
diff mbox series

Patch

diff --git a/drivers/fastboot/fb_getvar.c b/drivers/fastboot/fb_getvar.c
index 4268628f5e..4aa2f88ece 100644
--- a/drivers/fastboot/fb_getvar.c
+++ b/drivers/fastboot/fb_getvar.c
@@ -81,6 +81,47 @@  static const struct {
 	}
 };
 
+#if CONFIG_IS_ENABLED(FASTBOOT_FLASH)
+/**
+ * Get partition number and size for any storage type.
+ *
+ * Can be used to check if partition with specified name exists.
+ *
+ * If error occurs, this function guarantees to fill @p response with fail
+ * string. @p response can be rewritten in caller, if needed.
+ *
+ * @param[in] part_name Info for which partition name to look for
+ * @param[in,out] response Pointer to fastboot response buffer
+ * @param[out] size If not NULL, will contain partition size (in blocks)
+ * @return Partition number or negative value on error
+ */
+static int getvar_get_part_info(const char *part_name, char *response,
+				size_t *size)
+{
+	int r;
+# if CONFIG_IS_ENABLED(FASTBOOT_FLASH_MMC)
+	struct blk_desc *dev_desc;
+	disk_partition_t part_info;
+
+	r = fastboot_mmc_get_part_info(part_name, &dev_desc, &part_info,
+				       response);
+	if (r >= 0 && size)
+		*size = part_info.size;
+# elif CONFIG_IS_ENABLED(FASTBOOT_FLASH_NAND)
+	struct part_info *part_info;
+
+	r = fastboot_nand_get_part_info(part_name, &part_info, response);
+	if (r >= 0 && size)
+		*size = part_info->size;
+# else
+	fastboot_fail("this storage is not supported in bootloader", response);
+	r = -ENODEV;
+# endif
+
+	return r;
+}
+#endif
+
 static void getvar_version(char *var_parameter, char *response)
 {
 	fastboot_okay(FASTBOOT_VERSION, response);
@@ -176,22 +217,7 @@  static void getvar_partition_size(char *part_name, char *response)
 	int r;
 	size_t size;
 
-#if CONFIG_IS_ENABLED(FASTBOOT_FLASH_MMC)
-	struct blk_desc *dev_desc;
-	disk_partition_t part_info;
-
-	r = fastboot_mmc_get_part_info(part_name, &dev_desc, &part_info,
-				       response);
-	if (r >= 0)
-		size = part_info.size;
-#endif
-#if CONFIG_IS_ENABLED(FASTBOOT_FLASH_NAND)
-	struct part_info *part_info;
-
-	r = fastboot_nand_get_part_info(part_name, &part_info, response);
-	if (r >= 0)
-		size = part_info->size;
-#endif
+	r = getvar_get_part_info(part_name, response, &size);
 	if (r >= 0)
 		fastboot_response("OKAY", response, "0x%016zx", size);
 }