diff options
author | Bryn M. Reeves <bmr@redhat.com> | 2016-09-27 17:46:52 +0100 |
---|---|---|
committer | Bryn M. Reeves <bmr@redhat.com> | 2016-09-27 17:58:05 +0100 |
commit | 56c90ffa5e7d7c69c5230ac3bbee3ed03e7bae62 (patch) | |
tree | 334df00699e15aeba61a10c1a6fd9fa87d25d958 | |
parent | 6ec8854fdb051b092d5e262dc6c6d4c2ea075cd1 (diff) | |
download | lvm2-56c90ffa5e7d7c69c5230ac3bbee3ed03e7bae62.tar.gz |
libdm: fix dm_stats_delete_region() backwards compat
The dm_stats_delete_region() call removes a region from the bound
device, and, if the region is grouped, from the group leader
group descriptor stored in aux_data.
To do this requires a listed handle: previous versions of the
library do not since no dependencies exist between regions without
grouping.
This leads to strange behaviour when a command built against an old
version of the library is used with one supporting groups. Deleting
a region with dmstats succeeds, but logs errors:
# dmstats list
Name RgID RgSta RgSiz #Areas ArSize ProgID
vg_hex-root 0 0 1.00g 1 1.00g dmstats
vg_hex-root 1 1.00g 1.00g 1 1.00g dmstats
vg_hex-root 2 2.00g 1.00g 1 1.00g dmstats
# dmstats delete --regionid 2 vg_hex/root
Region ID 2 does not exist
Could not delete statistics region.
Command failed
# dmstats list
Name RgID RgSta RgSiz #Areas ArSize ProgID
vg_hex-root 0 0 1.00g 1 1.00g dmstats
vg_hex-root 1 1.00g 1.00g 1 1.00g dmstats
This happens because the call to dm_stats_delete_region() is inside
a dm_stats_walk_*() iterator: upon entry to the call, the iterator
is at its end conditions and about to terminate. Due to the call to
dm_stats_list() inside the function, it returns with an iterator at
the beginning of a walk and performs a further iteration before
exiting. This final loop makes a further attempt to delete the
(already deleted) region, leading to the confusing error messages.
-rw-r--r-- | WHATS_NEW_DM | 1 | ||||
-rw-r--r-- | libdm/libdm-stats.c | 48 |
2 files changed, 40 insertions, 9 deletions
diff --git a/WHATS_NEW_DM b/WHATS_NEW_DM index b048f7a98..ed0730798 100644 --- a/WHATS_NEW_DM +++ b/WHATS_NEW_DM @@ -1,5 +1,6 @@ Version 1.02.136 - ====================================== + Fix 'dmstats delete' with dmsetup older than v1.02.129 Fix stats walk segfault with dmsetup older than v1.02.129 Version 1.02.135 - 26th September 2016 diff --git a/libdm/libdm-stats.c b/libdm/libdm-stats.c index 75e014435..a033861ea 100644 --- a/libdm/libdm-stats.c +++ b/libdm/libdm-stats.c @@ -1550,7 +1550,7 @@ out: int dm_stats_walk_end(struct dm_stats *dms) { - if (!dms || !dms->regions) + if (!dms) return 1; if (_stats_walk_end(dms, &dms->cur_flags, @@ -2020,26 +2020,47 @@ int dm_stats_delete_region(struct dm_stats *dms, uint64_t region_id) { char msg[STATS_MSG_BUF_LEN]; struct dm_task *dmt; + int listed = 0; if (!_stats_bound(dms)) return_0; - if (!dms->regions && !dm_stats_list(dms, dms->program_id)) { + /* + * To correctly delete a region, that may be part of a group, a + * listed handle is required, since the region may need to be + * removed from another region's group descriptor; earlier + * versions of the region deletion interface do not have this + * requirement since there are no dependencies between regions. + * + * Listing a previously unlisted handle has numerous + * side-effects on other calls and operations (e.g. stats + * walks), especially when returning to a function that depends + * on the state of the region table, or statistics cursor. + * + * To avoid changing the semantics of the API, and the need for + * a versioned symbol, maintain a flag indicating when a listing + * has been carried out, and drop the region table before + * returning. + * + * This ensures compatibility with programs compiled against + * earlier versions of libdm. + */ + if (!dms->regions && !(listed = dm_stats_list(dms, dms->program_id))) { log_error("Could not obtain region list while deleting " "region ID " FMTu64, region_id); - return 0; + goto bad; } if (!dm_stats_get_nr_areas(dms)) { log_error("Could not delete region ID " FMTu64 ": " "no regions found", region_id); - return 0; + goto bad; } /* includes invalid and special region_id values */ if (!dm_stats_region_present(dms, region_id)) { log_error("Region ID " FMTu64 " does not exist", region_id); - return 0; + goto bad; } if(_stats_region_is_grouped(dms, region_id)) @@ -2047,12 +2068,12 @@ int dm_stats_delete_region(struct dm_stats *dms, uint64_t region_id) log_error("Could not remove region ID " FMTu64 " from " "group ID " FMTu64, region_id, dms->regions[region_id].group_id); - return 0; + goto bad; } if (!dm_snprintf(msg, sizeof(msg), "@stats_delete " FMTu64, region_id)) { log_error("Could not prepare @stats_delete message."); - return 0; + goto bad; } dmt = _stats_send_message(dms, msg); @@ -2060,10 +2081,19 @@ int dm_stats_delete_region(struct dm_stats *dms, uint64_t region_id) return_0; dm_task_destroy(dmt); - /* wipe region and mark as not present */ - _stats_region_destroy(&dms->regions[region_id]); + if (!listed) + /* wipe region and mark as not present */ + _stats_region_destroy(&dms->regions[region_id]); + else + /* return handle to prior state */ + _stats_regions_destroy(dms); return 1; +bad: + if (listed) + _stats_regions_destroy(dms); + + return 0; } int dm_stats_clear_region(struct dm_stats *dms, uint64_t region_id) |