[PATCH 0/3] Additional error checking for barebox_update

[PATCH 0/3] 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!

Thanks,
Andrey Smirnov

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

Andrey Smirnov (3):
  bbu: Expose bbu_find_handler_by_*() functions
  bbu: command: Make sure specified update handler exists
  bbu: Simplify bbu_find_handler_by_device()

 commands/barebox-update.c       | 36 +++++++++++++++++++++++++++++--
 common/bbu.c                    | 38 +++++++++------------------------
 drivers/usb/gadget/f_fastboot.c |  2 +-
 include/bbu.h                   |  3 ++-
 4 files changed, 47 insertions(+), 32 deletions(-)

-- 
2.17.1


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

[PATCH 1/3] 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 2/3] 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 | 36 ++++++++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/commands/barebox-update.c b/commands/barebox-update.c
index 84798ab0d..c5c11c27e 100644
--- a/commands/barebox-update.c
+++ b/commands/barebox-update.c
@@ -24,6 +24,12 @@
 #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;
@@ -46,8 +52,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;
@@ -57,6 +62,30 @@ 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 &&
+	    !bbu_find_handler_by_name(data.handler_name)) {
+		printf("handler '%s' does not exist\n",
+		       data.handler_name);
+		goto error;
+	} else if (data.devicefile &&
+		   !bbu_find_handler_by_device(data.devicefile)) {
+		printf("handler for '%s' does not exist\n",
+		       data.devicefile);
+		goto error;
+	} if (!data.handler_name &&
+	      !data.devicefile &&
+	      !bbu_find_handler_by_name(NULL)) {
+		printf("default handler does not exist\n");
+		goto error;
+	}
+
 	if (argc - optind > 0) {
 		data.imagefile = argv[optind];
 
@@ -74,6 +103,9 @@ static int do_barebox_update(int argc, char *argv[])
 	free(image);
 
 	return ret;
+error:
+	print_handlers_list();
+	return COMMAND_ERROR;
 }
 
 BAREBOX_CMD_HELP_START(barebox_update)
-- 
2.17.1


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

[PATCH 3/3] 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 fabd94966..7b65fe45c 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 2/3] bbu: command: Make sure specified update handler exists

[Sascha Hauer]

Hi Andrey,

On Wed, Sep 26, 2018 at 09:11:28PM -0700, Andrey Smirnov wrote:
> @@ -57,6 +62,30 @@ 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 &&
> +	    !bbu_find_handler_by_name(data.handler_name)) {
> +		printf("handler '%s' does not exist\n",
> +		       data.handler_name);
> +		goto error;
> +	} else if (data.devicefile &&
> +		   !bbu_find_handler_by_device(data.devicefile)) {
> +		printf("handler for '%s' does not exist\n",
> +		       data.devicefile);
> +		goto error;
> +	} if (!data.handler_name &&
> +	      !data.devicefile &&
> +	      !bbu_find_handler_by_name(NULL)) {
> +		printf("default handler does not exist\n");
> +		goto error;
> +	}

There should be a linebreak before the last if().

Maybe it should be rewritten as:

	if (data.handler_name)
		handler = bbu_find_handler_by_name();
	else if (data.devicefile)
		handler = bbu_find_handler_by_device();
	else
		handler = bbu_find_handler_by_name();

barebox_update() currently repeats these steps, so I think it would be a
next logical step to pass this handler to barebox_update() instead of
searching it there again.

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

Re: [PATCH 2/3] bbu: command: Make sure specified update handler exists

[Andrey Smirnov]

On Thu, Sep 27, 2018 at 4:42 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> Hi Andrey,
>
> On Wed, Sep 26, 2018 at 09:11:28PM -0700, Andrey Smirnov wrote:
> > @@ -57,6 +62,30 @@ 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 &&
> > +         !bbu_find_handler_by_name(data.handler_name)) {
> > +             printf("handler '%s' does not exist\n",
> > +                    data.handler_name);
> > +             goto error;
> > +     } else if (data.devicefile &&
> > +                !bbu_find_handler_by_device(data.devicefile)) {
> > +             printf("handler for '%s' does not exist\n",
> > +                    data.devicefile);
> > +             goto error;
> > +     } if (!data.handler_name &&
> > +           !data.devicefile &&
> > +           !bbu_find_handler_by_name(NULL)) {
> > +             printf("default handler does not exist\n");
> > +             goto error;
> > +     }
>
> There should be a linebreak before the last if().
>
> Maybe it should be rewritten as:
>
>         if (data.handler_name)
>                 handler = bbu_find_handler_by_name();
>         else if (data.devicefile)
>                 handler = bbu_find_handler_by_device();
>         else
>                 handler = bbu_find_handler_by_name();
>
> barebox_update() currently repeats these steps, so I think it would be a
> next logical step to pass this handler to barebox_update() instead of
> searching it there again.
>

Yeah, makes sense. Will change in v2.

Thanks,
Andrey Smirnov

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