From 4eb04c8c05e52776891f62863375ceacf866de77 Mon Sep 17 00:00:00 2001 From: David Teigland Date: Tue, 22 Feb 2022 15:03:11 -0600 Subject: [PATCH] devices: fix dev_name assumptions dev_name(dev) returns "[unknown]" if there are no names on dev->aliases. It's meant mainly for log messages. Many places assume a valid path name is returned, and use it directly. A caller that wants to use the path from dev_name() must first check if the dev has any paths with dm_list_empty(&dev->aliases). --- lib/activate/dev_manager.c | 9 ++++++++- lib/device/dev-type.c | 3 +++ lib/device/device_id.c | 13 +++++++++++-- lib/label/hints.c | 2 ++ lib/label/label.c | 23 ++++++++++++++++++++++- lib/locking/lvmlockd.c | 4 ++++ lib/metadata/mirror.c | 17 +++++++++++++---- lib/metadata/pv_list.c | 5 +++++ lib/metadata/vg.c | 5 +++++ test/shell/losetup-partscan.sh | 2 ++ 10 files changed, 75 insertions(+), 8 deletions(-) diff --git a/lib/activate/dev_manager.c b/lib/activate/dev_manager.c index 63bfd9b74..2cae3bed1 100644 --- a/lib/activate/dev_manager.c +++ b/lib/activate/dev_manager.c @@ -2947,6 +2947,10 @@ int add_areas_line(struct dev_manager *dm, struct lv_segment *seg, /* FIXME Avoid repeating identical stat in dm_tree_node_add_target_area */ for (s = start_area; s < areas; s++) { + + /* FIXME: dev_name() does not return NULL! It needs to check if dm_list_empty(&dev->aliases) + but this knot of logic is too complex to pull apart without careful deconstruction. */ + if ((seg_type(seg, s) == AREA_PV && (!seg_pvseg(seg, s) || !seg_pv(seg, s) || !seg_dev(seg, s) || !(name = dev_name(seg_dev(seg, s))) || !*name || @@ -2965,7 +2969,10 @@ int add_areas_line(struct dev_manager *dm, struct lv_segment *seg, return_0; num_error_areas++; } else if (seg_type(seg, s) == AREA_PV) { - if (!dm_tree_node_add_target_area(node, dev_name(seg_dev(seg, s)), NULL, + struct device *dev = seg_dev(seg, s); + name = dm_list_empty(&dev->aliases) ? NULL : dev_name(dev); + + if (!dm_tree_node_add_target_area(node, name, NULL, (seg_pv(seg, s)->pe_start + (extent_size * seg_pe(seg, s))))) return_0; num_existing_areas++; diff --git a/lib/device/dev-type.c b/lib/device/dev-type.c index 0e77a009d..c67a86fa3 100644 --- a/lib/device/dev-type.c +++ b/lib/device/dev-type.c @@ -966,6 +966,9 @@ static int _wipe_known_signatures_with_blkid(struct device *dev, const char *nam /* TODO: Should we check for valid dev - _dev_is_valid(dev)? */ + if (dm_list_empty(&dev->aliases)) + goto_out; + if (!(probe = blkid_new_probe_from_filename(dev_name(dev)))) { log_error("Failed to create a new blkid probe for device %s.", dev_name(dev)); goto out; diff --git a/lib/device/device_id.c b/lib/device/device_id.c index c8df47345..7ce955b11 100644 --- a/lib/device/device_id.c +++ b/lib/device/device_id.c @@ -347,6 +347,8 @@ const char *device_id_system_read(struct cmd_context *cmd, struct device *dev, u } else if (idtype == DEV_ID_TYPE_DEVNAME) { + if (dm_list_empty(&dev->aliases)) + goto_bad; if (!(idname = strdup(dev_name(dev)))) goto_bad; return idname; @@ -955,6 +957,10 @@ int device_id_add(struct cmd_context *cmd, struct device *dev, const char *pvid_ if (!dev_get_partition_number(dev, &part)) return_0; + /* Ensure valid dev_name(dev) below. */ + if (dm_list_empty(&dev->aliases)) + return_0; + /* * When enable_devices_file=0 and pending_devices_file=1 we let * pvcreate/vgcreate add new du's to cmd->use_devices. These du's may @@ -1842,6 +1848,9 @@ void device_ids_validate(struct cmd_context *cmd, struct dm_list *scanned_devs, if (dev->flags & DEV_SCAN_NOT_READ) continue; + if (dm_list_empty(&dev->aliases)) + continue; + if (!cmd->filter->passes_filter(cmd, cmd->filter, dev, "persistent")) { log_warn("Devices file %s is excluded by filter: %s.", dev_name(dev), dev_filtered_reason(dev)); @@ -2225,14 +2234,14 @@ void device_ids_find_renamed_devs(struct cmd_context *cmd, struct dm_list *dev_l dm_list_iterate_items(dil, &search_pvids) { char *dup_devname1, *dup_devname2, *dup_devname3; - if (!dil->dev) { + if (!dil->dev || dm_list_empty(&dil->dev->aliases)) { not_found++; continue; } - found++; dev = dil->dev; devname = dev_name(dev); + found++; if (!(du = get_du_for_pvid(cmd, dil->pvid))) { /* shouldn't happen */ diff --git a/lib/label/hints.c b/lib/label/hints.c index 35ae7f5cc..edce6f517 100644 --- a/lib/label/hints.c +++ b/lib/label/hints.c @@ -500,6 +500,8 @@ int validate_hints(struct cmd_context *cmd, struct dm_list *hints) if (!(iter = dev_iter_create(NULL, 0))) return 0; while ((dev = dev_iter_get(cmd, iter))) { + if (dm_list_empty(&dev->aliases)) + continue; if (!(hint = _find_hint_name(hints, dev_name(dev)))) continue; diff --git a/lib/label/label.c b/lib/label/label.c index 66d6e7db7..ffb393891 100644 --- a/lib/label/label.c +++ b/lib/label/label.c @@ -1130,6 +1130,7 @@ int label_scan_vg_online(struct cmd_context *cmd, const char *vgname, * sure to find the device. */ if (try_dev_scan) { + log_debug("Repeat dev cache scan to translate devnos."); dev_cache_scan(cmd); dm_list_iterate_items(po, &pvs_online) { if (po->dev) @@ -1736,6 +1737,12 @@ void label_scan_invalidate_lvs(struct cmd_context *cmd, struct dm_list *lvs) struct lv_list *lvl; dev_t devt; + /* + * FIXME: this is all unnecessary unless there are PVs stacked on LVs, + * so we can skip all of this if scan_lvs=0. + */ + log_debug("invalidating devs for any pvs on lvs"); + if (get_device_list(NULL, &devs, &devs_features)) { if (devs_features & DM_DEVICE_LIST_HAS_UUID) { dm_list_iterate_items(dm_dev, devs) @@ -1879,10 +1886,24 @@ int label_scan_open_rw(struct device *dev) int label_scan_reopen_rw(struct device *dev) { + const char *name; int flags = 0; int prev_fd = dev->bcache_fd; int fd; + if (dm_list_empty(&dev->aliases)) { + log_error("Cannot reopen rw device %d:%d with no valid paths di %d fd %d.", + (int)MAJOR(dev->dev), (int)MINOR(dev->dev), dev->bcache_di, dev->bcache_fd); + return 0; + } + + name = dev_name(dev); + if (!name || name[0] != '/') { + log_error("Cannot reopen rw device %d:%d with no valid name di %d fd %d.", + (int)MAJOR(dev->dev), (int)MINOR(dev->dev), dev->bcache_di, dev->bcache_fd); + return 0; + } + if (!(dev->flags & DEV_IN_BCACHE)) { if ((dev->bcache_fd != -1) || (dev->bcache_di != -1)) { /* shouldn't happen */ @@ -1912,7 +1933,7 @@ int label_scan_reopen_rw(struct device *dev) flags |= O_NOATIME; flags |= O_RDWR; - fd = open(dev_name(dev), flags, 0777); + fd = open(name, flags, 0777); if (fd < 0) { log_error("Failed to open rw %s errno %d di %d fd %d.", dev_name(dev), errno, dev->bcache_di, dev->bcache_fd); diff --git a/lib/locking/lvmlockd.c b/lib/locking/lvmlockd.c index b598df3d6..60c80f1b1 100644 --- a/lib/locking/lvmlockd.c +++ b/lib/locking/lvmlockd.c @@ -272,6 +272,8 @@ static void _lockd_retrive_vg_pv_list(struct volume_group *vg, i = 0; dm_list_iterate_items(pvl, &vg->pvs) { + if (!pvl->pv->dev || dm_list_empty(&pvl->pv->dev->aliases)) + continue; lock_pvs->path[i] = strdup(pv_dev_name(pvl->pv)); if (!lock_pvs->path[i]) { log_error("Fail to allocate PV path for VG %s", vg->name); @@ -341,6 +343,8 @@ static void _lockd_retrive_lv_pv_list(struct volume_group *vg, dm_list_iterate_items(pvl, &vg->pvs) { if (lv_is_on_pv(lv, pvl->pv)) { + if (!pvl->pv->dev || dm_list_empty(&pvl->pv->dev->aliases)) + continue; lock_pvs->path[i] = strdup(pv_dev_name(pvl->pv)); if (!lock_pvs->path[i]) { log_error("Fail to allocate PV path for LV %s/%s", diff --git a/lib/metadata/mirror.c b/lib/metadata/mirror.c index e2bf191a1..46da57948 100644 --- a/lib/metadata/mirror.c +++ b/lib/metadata/mirror.c @@ -1231,14 +1231,23 @@ int remove_mirrors_from_segments(struct logical_volume *lv, const char *get_pvmove_pvname_from_lv_mirr(const struct logical_volume *lv_mirr) { struct lv_segment *seg; + struct device *dev; dm_list_iterate_items(seg, &lv_mirr->segments) { if (!seg_is_mirrored(seg)) continue; - if (seg_type(seg, 0) == AREA_PV) - return dev_name(seg_dev(seg, 0)); - if (seg_type(seg, 0) == AREA_LV) - return dev_name(seg_dev(first_seg(seg_lv(seg, 0)), 0)); + if (seg_type(seg, 0) == AREA_PV) { + dev = seg_dev(seg, 0); + if (!dev || dm_list_empty(&dev->aliases)) + return NULL; + return dev_name(dev); + } + if (seg_type(seg, 0) == AREA_LV) { + dev = seg_dev(first_seg(seg_lv(seg, 0)), 0); + if (!dev || dm_list_empty(&dev->aliases)) + return NULL; + return dev_name(dev); + } } return NULL; diff --git a/lib/metadata/pv_list.c b/lib/metadata/pv_list.c index 813e8e525..fc3667db0 100644 --- a/lib/metadata/pv_list.c +++ b/lib/metadata/pv_list.c @@ -152,6 +152,11 @@ static int _create_pv_entry(struct dm_pool *mem, struct pv_list *pvl, struct pv_list *new_pvl = NULL, *pvl2; struct dm_list *pe_ranges; + if (!pvl->pv->dev || dm_list_empty(&pvl->pv->dev->aliases)) { + log_error("Failed to create PV entry for missing device."); + return 0; + } + pvname = pv_dev_name(pvl->pv); if (allocatable_only && !(pvl->pv->status & ALLOCATABLE_PV)) { log_warn("WARNING: Physical volume %s not allocatable.", pvname); diff --git a/lib/metadata/vg.c b/lib/metadata/vg.c index 85482552a..adc954bab 100644 --- a/lib/metadata/vg.c +++ b/lib/metadata/vg.c @@ -679,6 +679,11 @@ int vgreduce_single(struct cmd_context *cmd, struct volume_group *vg, return r; } + if (!pv->dev || dm_list_empty(&pv->dev->aliases)) { + log_error("No device found for PV."); + return r; + } + log_debug("vgreduce_single VG %s PV %s", vg->name, pv_dev_name(pv)); if (pv_pe_alloc_count(pv)) { diff --git a/test/shell/losetup-partscan.sh b/test/shell/losetup-partscan.sh index 99f552ad1..670568945 100644 --- a/test/shell/losetup-partscan.sh +++ b/test/shell/losetup-partscan.sh @@ -33,6 +33,8 @@ aux udev_wait ls -la "${LOOP}"* test -e "${LOOP}p1" +aux lvmconf 'devices/scan = "/dev"' + aux extend_filter "a|$LOOP|" aux extend_devices "$LOOP"