[PATCH] Recognise storage devices on USB bus
Samuel Mendoza-Jonas
sam.mj at au1.ibm.com
Tue Aug 25 10:32:20 AEST 2015
On 25/08/15 10:13, Cyril Bur wrote:
> 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.
>
Yeah this is starting to get a little messy, I'm thinking I'll have a look at the
function as a whole as see if we can streamline the checking a bit.
>> !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 ;)
>
*cough*
>> 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:
--
-----------
LTC Ozlabs
IBM
More information about the Petitboot
mailing list