[PATCH] Recognise storage devices on USB bus

Tue Aug 25 10:13:10 AEST 2015

On Mon, 24 Aug 2015 15:19:57 +1000
Samuel Mendoza-Jonas <sam.mj at au1.ibm.com> wrote:

> Users may want to prioritise USB-attached storage devices differently to
> other devices. Detect if a device is USB-attached and add a new device
> type to identify it.
> 

Hey Sam,

No issues just feedback.

Reviewed-by: Cyril Bur <cyril.bur at au1.ibm.com>

> Signed-off-by: Samuel Mendoza-Jonas <sam.mj at au1.ibm.com>
> ---
>  discover/device-handler.c | 1 +
>  discover/udev.c           | 9 +++++++--
>  lib/types/types.c         | 6 ++++++
>  lib/types/types.h         | 1 +
>  ui/ncurses/nc-config.c    | 2 +-
>  ui/ncurses/nc-menu.c      | 6 ++++--
>  ui/test/discover-test.c   | 2 ++
>  7 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/discover/device-handler.c b/discover/device-handler.c
> index 4f7a7b7..9246f0d 100644
> --- a/discover/device-handler.c
> +++ b/discover/device-handler.c
> @@ -457,6 +457,7 @@ struct {
>  } device_type_map[] = {
>  	{ IPMI_BOOTDEV_NETWORK, DEVICE_TYPE_NETWORK },
>  	{ IPMI_BOOTDEV_DISK, DEVICE_TYPE_DISK },
> +	{ IPMI_BOOTDEV_DISK, DEVICE_TYPE_USB },
>  	{ IPMI_BOOTDEV_CDROM, DEVICE_TYPE_OPTICAL },
>  };
>  
> diff --git a/discover/udev.c b/discover/udev.c
> index 87a3748..d1815df 100644
> --- a/discover/udev.c
> +++ b/discover/udev.c
> @@ -167,12 +167,17 @@ static int udev_handle_block_add(struct pb_udev *udev, struct udev_device *dev,
>  	prop = udev_device_get_property_value(dev, "ID_FS_LABEL");
>  	if (prop)
>  		ddev->label = talloc_strdup(ddev, prop);
> -	ddev->device->type = cdrom ? DEVICE_TYPE_OPTICAL : DEVICE_TYPE_DISK;
> +	prop = udev_device_get_property_value(dev, "ID_BUS");
> +	if (prop && !strncmp(prop, "usb", strlen("usb")))
> +		ddev->device->type = DEVICE_TYPE_USB;
> +	else
> +		ddev->device->type = cdrom ? DEVICE_TYPE_OPTICAL : DEVICE_TYPE_DISK;
>  
>  	udev_setup_device_params(dev, ddev);
>  
>  	/* Create a snapshot for all disks, unless it is an assembled RAID array */
> -	if (ddev->device->type == DEVICE_TYPE_DISK &&
> +	if ((ddev->device->type == DEVICE_TYPE_DISK ||
> +	     ddev->device->type == DEVICE_TYPE_USB) &&

I wonder about the usefulness of a helper function for this condition? It only
crops up once and it isn't ridiculous yet. Maybe if there's ever another
DEVICE_TYPE_* that has similar properties, or if that condition starts cropping
up in other places... it will be worth it.

>  	    !udev_device_get_property_value(dev, "MD_LEVEL"))
>  		devmapper_init_snapshot(udev->handler, ddev);
>  
> diff --git a/lib/types/types.c b/lib/types/types.c
> index 95a3a48..611f2a1 100644
> --- a/lib/types/types.c
> +++ b/lib/types/types.c
> @@ -27,6 +27,8 @@ const char *device_type_display_name(enum device_type type)
>  	switch (type) {
>  	case DEVICE_TYPE_DISK:
>  		return _("Disk");
> +	case DEVICE_TYPE_USB:
> +		return _("USB");
>  	case DEVICE_TYPE_OPTICAL:
>  		return _("Optical");
>  	case DEVICE_TYPE_NETWORK:
> @@ -44,6 +46,8 @@ const char *device_type_name(enum device_type type)
>  	switch (type) {
>  	case DEVICE_TYPE_DISK:
>  		return "disk";
> +	case DEVICE_TYPE_USB:
> +		return "usb";
>  	case DEVICE_TYPE_OPTICAL:
>  		return "optical";
>  	case DEVICE_TYPE_NETWORK:
> @@ -60,6 +64,8 @@ enum device_type find_device_type(const char *str)
>  {
>  	if (!strncmp(str, "disk", strlen("disk")))
>  		return DEVICE_TYPE_DISK;
> +	if (!strncmp(str, "usb", strlen("usb")))
> +		return DEVICE_TYPE_USB;
>  	if (!strncmp(str, "optical", strlen("optical")))
>  		return DEVICE_TYPE_OPTICAL;
>  	if (!strncmp(str, "network", strlen("network")))
> diff --git a/lib/types/types.h b/lib/types/types.h
> index 0415206..6a2c258 100644
> --- a/lib/types/types.h
> +++ b/lib/types/types.h
> @@ -8,6 +8,7 @@
>  enum device_type {
>  	DEVICE_TYPE_NETWORK,
>  	DEVICE_TYPE_DISK,
> +	DEVICE_TYPE_USB,
>  	DEVICE_TYPE_OPTICAL,
>  	DEVICE_TYPE_ANY,
>  	DEVICE_TYPE_UNKNOWN,
> diff --git a/ui/ncurses/nc-config.c b/ui/ncurses/nc-config.c
> index 6363bb9..f7c6b8c 100644
> --- a/ui/ncurses/nc-config.c
> +++ b/ui/ncurses/nc-config.c
> @@ -762,7 +762,7 @@ static void config_screen_setup_widgets(struct config_screen *screen,
>  		widget_subset_add_option(screen->widgets.boot_order_f, label);
>  	}
>  
> -	for (i = DEVICE_TYPE_NETWORK; i < DEVICE_TYPE_NETWORK + 4; i++) {
> +	for (i = DEVICE_TYPE_NETWORK; i < DEVICE_TYPE_UNKNOWN; i++) {

One wonders why this wasn't already the case ;)

>  		char *label;
>  
>  		if (i == DEVICE_TYPE_ANY)
> diff --git a/ui/ncurses/nc-menu.c b/ui/ncurses/nc-menu.c
> index b8f9a35..b42dc23 100644
> --- a/ui/ncurses/nc-menu.c
> +++ b/ui/ncurses/nc-menu.c
> @@ -253,6 +253,7 @@ struct pmenu_item *pmenu_find_device(struct pmenu *menu, struct device *dev,
>  	switch (dev->type) {
>  	case DEVICE_TYPE_OPTICAL:
>  	case DEVICE_TYPE_DISK:
> +	case DEVICE_TYPE_USB:
>  		/* Find block info */
>  		for (i = 0; sys && i < sys->n_blockdevs; i++) {
>  			bd = sys->blockdevs[i];
> @@ -263,8 +264,9 @@ struct pmenu_item *pmenu_find_device(struct pmenu *menu, struct device *dev,
>  		}
>  		if (matched) {
>  			snprintf(buf,sizeof(buf),"[%s: %s / %s]",
> -				dev->type == DEVICE_TYPE_DISK ?
> -					_("Disk") : _("CD/DVD"),
> +				dev->type == DEVICE_TYPE_OPTICAL ?
> +				_("CD/DVD") :
> +				device_type_display_name(dev->type),
>  				bd->name, bd->uuid);

So, as discussed, putting the _("CD/DVD") bit inside device_type_display_name
might be nice but since it would cause a change in string representation for
DEVICE_TYPE_OPTICAL in places, best left to a separate patch

>  		}
>  		break;
> diff --git a/ui/test/discover-test.c b/ui/test/discover-test.c
> index 363a289..b099b59 100644
> --- a/ui/test/discover-test.c
> +++ b/ui/test/discover-test.c
> @@ -8,6 +8,8 @@ static const char *device_type_string(enum device_type type)
>  	switch (type) {
>  	case DEVICE_TYPE_DISK:
>  		return "disk";
> +	case DEVICE_TYPE_USB:
> +		return "usb";
>  	case DEVICE_TYPE_NETWORK:
>  		return "network";
>  	case DEVICE_TYPE_OPTICAL: