[PATCH v2 0/4] Additional error checking for barebox_update

[PATCH v2 0/4] Additional error checking for barebox_update

[Andrey Smirnov]

Everyone:

This is a follow up to discussion [1]. Hopefull all of the patches are
self-explanatory enough.

Feedback is welcome!

Changes since [v1]:

    - Move handler lookup out of barebox_update() in order to avoid
      making calls to bbu_handler_by_*() twice

Thanks,
Andrey Smirnov

[v1] http://lists.infradead.org/pipermail/barebox/2018-September/034865.html
[1] http://lists.infradead.org/pipermail/barebox/2018-August/034573.html

Andrey Smirnov (4):
  bbu: Expose bbu_find_handler_by_*() functions
  bbu: Add "handler" parameter to barebox_update()
  bbu: command: Make sure specified update handler exists
  bbu: Simplify bbu_find_handler_by_device()

 commands/barebox-update.c       | 41 +++++++++++++++++++++++++---
 common/bbu.c                    | 47 +++++++--------------------------
 drivers/usb/gadget/f_fastboot.c |  6 +++--
 include/bbu.h                   |  5 ++--
 4 files changed, 55 insertions(+), 44 deletions(-)

-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 1/4] bbu: Expose bbu_find_handler_by_*() functions

[Andrey Smirnov]

Expose bbu_find_handler_by_device() and bbu_find_handler_by_name() as
public functions and convert the only user of
barebox_update_handler_exists() to use the former function instead.

With this done, barebox_update_handler_exists() is no longer used
anywhere in the code and can be removed.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 common/bbu.c                    | 24 +++++-------------------
 drivers/usb/gadget/f_fastboot.c |  2 +-
 include/bbu.h                   |  3 ++-
 3 files changed, 8 insertions(+), 21 deletions(-)

diff --git a/common/bbu.c b/common/bbu.c
index 3974bf672..fabd94966 100644
--- a/common/bbu.c
+++ b/common/bbu.c
@@ -99,7 +99,7 @@ int bbu_confirm(struct bbu_data *data)
 	return -EINTR;
 }
 
-static struct bbu_handler *bbu_find_handler(const char *name)
+struct bbu_handler *bbu_find_handler_by_name(const char *name)
 {
 	struct bbu_handler *handler;
 
@@ -117,7 +117,7 @@ static struct bbu_handler *bbu_find_handler(const char *name)
 	return NULL;
 }
 
-static struct bbu_handler *bbu_find_handler_by_device(const char *devicepath)
+struct bbu_handler *bbu_find_handler_by_device(const char *devicepath)
 {
 	struct bbu_handler *handler;
 
@@ -140,20 +140,6 @@ static struct bbu_handler *bbu_find_handler_by_device(const char *devicepath)
 	return NULL;
 }
 
-bool barebox_update_handler_exists(struct bbu_data *data)
-{
-	struct bbu_handler *handler;
-
-	handler = bbu_find_handler_by_device(data->devicefile);
-	if (handler)
-		return true;
-
-	if (!data->handler_name)
-		return false;
-
-	return bbu_find_handler(data->handler_name) != NULL;
-}
-
 static int bbu_check_of_compat(struct bbu_data *data)
 {
 	struct device_node *root_node;
@@ -238,7 +224,7 @@ int barebox_update(struct bbu_data *data)
 	handler = bbu_find_handler_by_device(data->devicefile);
 
 	if (!handler)
-		handler = bbu_find_handler(data->handler_name);
+		handler = bbu_find_handler_by_name(data->handler_name);
 
 	if (!handler)
 		return -ENODEV;
@@ -292,11 +278,11 @@ void bbu_handlers_list(void)
  */
 int bbu_register_handler(struct bbu_handler *handler)
 {
-	if (bbu_find_handler(handler->name))
+	if (bbu_find_handler_by_name(handler->name))
 		return -EBUSY;
 
 	if (handler->flags & BBU_HANDLER_FLAG_DEFAULT &&
-			bbu_find_handler(NULL))
+	    bbu_find_handler_by_name(NULL))
 		return -EBUSY;
 
 	list_add_tail(&handler->list, &bbu_image_handlers);
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c
index 74fb524c1..ea36fc988 100644
--- a/drivers/usb/gadget/f_fastboot.c
+++ b/drivers/usb/gadget/f_fastboot.c
@@ -996,7 +996,7 @@ static void cb_flash(struct f_fastboot *f_fb, const char *cmd)
 			.flags = BBU_FLAG_YES,
 		};
 
-		if (!barebox_update_handler_exists(&data))
+		if (!bbu_find_handler_by_device(data.devicefile))
 			goto copy;
 
 		fastboot_tx_print(f_fb, "INFOThis is a barebox image...");
diff --git a/include/bbu.h b/include/bbu.h
index d1ab9f563..775d7a310 100644
--- a/include/bbu.h
+++ b/include/bbu.h
@@ -42,7 +42,8 @@ int bbu_confirm(struct bbu_data *);
 
 int barebox_update(struct bbu_data *);
 
-bool barebox_update_handler_exists(struct bbu_data *);
+struct bbu_handler *bbu_find_handler_by_name(const char *name);
+struct bbu_handler *bbu_find_handler_by_device(const char *devicepath);
 
 void bbu_handlers_list(void);
 
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 2/4] bbu: Add "handler" parameter to barebox_update()

[Andrey Smirnov]

Add "handler" parameter to barebox_update() and remove the code that
was respondible for header lookup before. With this change finding
appropriate handler is caller's responsibility, which makes it
possible to implement custom handler lookup/existence check, chache
it, and then re-use it without calling handler_find_by_* functions for
the second time.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 commands/barebox-update.c       | 11 ++++++++++-
 common/bbu.c                    | 11 +----------
 drivers/usb/gadget/f_fastboot.c |  6 ++++--
 include/bbu.h                   |  2 +-
 4 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/commands/barebox-update.c b/commands/barebox-update.c
index 84798ab0d..903b4068d 100644
--- a/commands/barebox-update.c
+++ b/commands/barebox-update.c
@@ -28,6 +28,7 @@ static int do_barebox_update(int argc, char *argv[])
 {
 	int opt, ret, repair = 0;
 	struct bbu_data data = {};
+	struct bbu_handler *handler;
 	void *image = NULL;
 
 	while ((opt = getopt(argc, argv, "t:yf:ld:r")) > 0) {
@@ -69,7 +70,15 @@ static int do_barebox_update(int argc, char *argv[])
 			return COMMAND_ERROR_USAGE;
 	}
 
-	ret = barebox_update(&data);
+	handler = bbu_find_handler_by_device(data.devicefile);
+
+	if (!handler)
+		handler = bbu_find_handler_by_name(data.handler_name);
+
+	if (!handler)
+		return COMMAND_ERROR_USAGE;
+
+	ret = barebox_update(&data, handler);
 
 	free(image);
 
diff --git a/common/bbu.c b/common/bbu.c
index fabd94966..75c3221d5 100644
--- a/common/bbu.c
+++ b/common/bbu.c
@@ -216,19 +216,10 @@ static int bbu_check_metadata(struct bbu_data *data)
 /*
  * do a barebox update with data from *data
  */
-int barebox_update(struct bbu_data *data)
+int barebox_update(struct bbu_data *data, struct bbu_handler *handler)
 {
-	struct bbu_handler *handler;
 	int ret;
 
-	handler = bbu_find_handler_by_device(data->devicefile);
-
-	if (!handler)
-		handler = bbu_find_handler_by_name(data->handler_name);
-
-	if (!handler)
-		return -ENODEV;
-
 	if (!data->image && !data->imagefile &&
 	    !(handler->flags & BBU_HANDLER_CAN_REFRESH)) {
 		pr_err("No Image file given\n");
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c
index ea36fc988..2c137fe7e 100644
--- a/drivers/usb/gadget/f_fastboot.c
+++ b/drivers/usb/gadget/f_fastboot.c
@@ -991,12 +991,14 @@ static void cb_flash(struct f_fastboot *f_fb, const char *cmd)
 	}
 
 	if (IS_ENABLED(CONFIG_BAREBOX_UPDATE) && filetype_is_barebox_image(filetype)) {
+		struct bbu_handler *handler;
 		struct bbu_data data = {
 			.devicefile = filename,
 			.flags = BBU_FLAG_YES,
 		};
 
-		if (!bbu_find_handler_by_device(data.devicefile))
+		handler = bbu_find_handler_by_device(data.devicefile);
+		if (!handler)
 			goto copy;
 
 		fastboot_tx_print(f_fb, "INFOThis is a barebox image...");
@@ -1015,7 +1017,7 @@ static void cb_flash(struct f_fastboot *f_fb, const char *cmd)
 		data.image = f_fb->buf;
 		data.imagefile = sourcefile;
 
-		ret = barebox_update(&data);
+		ret = barebox_update(&data, handler);
 
 		if (ret)
 			fastboot_tx_print(f_fb, "FAILupdate barebox: %s", strerror(-ret));
diff --git a/include/bbu.h b/include/bbu.h
index 775d7a310..0ed355b53 100644
--- a/include/bbu.h
+++ b/include/bbu.h
@@ -40,7 +40,7 @@ int bbu_force(struct bbu_data *, const char *fmt, ...)
 
 int bbu_confirm(struct bbu_data *);
 
-int barebox_update(struct bbu_data *);
+int barebox_update(struct bbu_data *, struct bbu_handler *);
 
 struct bbu_handler *bbu_find_handler_by_name(const char *name);
 struct bbu_handler *bbu_find_handler_by_device(const char *devicepath);
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 3/4] bbu: command: Make sure specified update handler exists

[Andrey Smirnov]

Add code to verify that update handler specified with either -t or of
-d exists before commencing the update procedure.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 commands/barebox-update.c | 46 ++++++++++++++++++++++++++++++---------
 1 file changed, 36 insertions(+), 10 deletions(-)

diff --git a/commands/barebox-update.c b/commands/barebox-update.c
index 903b4068d..53af2a851 100644
--- a/commands/barebox-update.c
+++ b/commands/barebox-update.c
@@ -24,12 +24,20 @@
 #include <bbu.h>
 #include <fs.h>
 
+static void print_handlers_list(void)
+{
+	printf("registered update handlers:\n");
+	bbu_handlers_list();
+}
+
 static int do_barebox_update(int argc, char *argv[])
 {
 	int opt, ret, repair = 0;
 	struct bbu_data data = {};
 	struct bbu_handler *handler;
 	void *image = NULL;
+	const char *name;
+	const char *fmt;
 
 	while ((opt = getopt(argc, argv, "t:yf:ld:r")) > 0) {
 		switch (opt) {
@@ -47,8 +55,7 @@ static int do_barebox_update(int argc, char *argv[])
 			data.flags |= BBU_FLAG_YES;
 			break;
 		case 'l':
-			printf("registered update handlers:\n");
-			bbu_handlers_list();
+			print_handlers_list();
 			return 0;
 		case 'r':
 			repair = 1;
@@ -58,6 +65,33 @@ static int do_barebox_update(int argc, char *argv[])
 		}
 	}
 
+	if (data.handler_name && data.devicefile) {
+		printf("Both TARGET and DEVICE are provided. "
+		       "Ignoring the latter\n");
+
+		data.devicefile = NULL;
+	}
+
+	if (data.handler_name) {
+		handler = bbu_find_handler_by_name(data.handler_name);
+		fmt = "handler '%s' does not exist\n";
+		name = data.handler_name;
+	} else if (data.devicefile) {
p+		handler = bbu_find_handler_by_device(data.devicefile);
+		fmt = "handler for '%s' does not exist\n";
+		name = data.devicefile;
+	} else {
+		handler = bbu_find_handler_by_name(NULL);
+		fmt = "default handler does not exist\n";
+		name = NULL;
+	}
+
+	if (!handler) {
+		printf(fmt, name);
+		print_handlers_list();
+		return COMMAND_ERROR;
+	}
+
 	if (argc - optind > 0) {
 		data.imagefile = argv[optind];
 
@@ -70,14 +104,6 @@ static int do_barebox_update(int argc, char *argv[])
 			return COMMAND_ERROR_USAGE;
 	}
 
-	handler = bbu_find_handler_by_device(data.devicefile);
-
-	if (!handler)
-		handler = bbu_find_handler_by_name(data.handler_name);
-
-	if (!handler)
-		return COMMAND_ERROR_USAGE;
-
 	ret = barebox_update(&data, handler);
 
 	free(image);
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 4/4] bbu: Simplify bbu_find_handler_by_device()

[Andrey Smirnov]

Simplify bbu_find_handler_by_device() by making use of
devpath_to_name() as well as some basic recursion to avoid coding the
same loop twice.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 common/bbu.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/common/bbu.c b/common/bbu.c
index 75c3221d5..5cb09e4eb 100644
--- a/common/bbu.c
+++ b/common/bbu.c
@@ -120,22 +120,18 @@ struct bbu_handler *bbu_find_handler_by_name(const char *name)
 struct bbu_handler *bbu_find_handler_by_device(const char *devicepath)
 {
 	struct bbu_handler *handler;
+	const char *devname = devpath_to_name(devicepath);
 
 	if (!devicepath)
 		return NULL;
 
-	list_for_each_entry(handler, &bbu_image_handlers, list)
+	list_for_each_entry(handler, &bbu_image_handlers, list) {
 		if (!strcmp(handler->devicefile, devicepath))
 			return handler;
+	}
 
-	if (strncmp(devicepath, "/dev/", 5))
-		return NULL;
-
-	devicepath += 5;
-
-	list_for_each_entry(handler, &bbu_image_handlers, list)
-		if (!strcmp(handler->devicefile, devicepath))
-			return handler;
+	if (devname != devicepath)
+		return bbu_find_handler_by_device(devname);
 
 	return NULL;
 }
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 0/4] Additional error checking for barebox_update

[Sascha Hauer]

On Sun, Oct 07, 2018 at 11:35:14PM -0700, Andrey Smirnov wrote:
> Everyone:
> 
> This is a follow up to discussion [1]. Hopefull all of the patches are
> self-explanatory enough.
> 
> Feedback is welcome!
> 
> Changes since [v1]:
> 
>     - Move handler lookup out of barebox_update() in order to avoid
>       making calls to bbu_handler_by_*() twice
> 
> Thanks,
> Andrey Smirnov
> 
> [v1] http://lists.infradead.org/pipermail/barebox/2018-September/034865.html
> [1] http://lists.infradead.org/pipermail/barebox/2018-August/034573.html
> 
> Andrey Smirnov (4):
>   bbu: Expose bbu_find_handler_by_*() functions
>   bbu: Add "handler" parameter to barebox_update()
>   bbu: command: Make sure specified update handler exists
>   bbu: Simplify bbu_find_handler_by_device()

Applied, thanks

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox