devices file: back up each version

Create backup copies of system.devices in /etc/lvm/devices/backup
named system.devices-YYYYMMDD.HHMMSS.NNNN.  NNNN is the version
counter from the file.

Each time that an lvm command writes a new system.devices file,
it also writes the same file in the backup directory.

A new comment line is added to system.devices with HASH=<num>
where <num> is a crc calculated from the uncommented lines in
system.devices.  This lets lvm detect if the file has been
modified outside of lvm itself.

If system.devices is edited directly, the next time a command
reads the file, the crc will not match the HASH value.  The
command will then rewrite system.devices with the correct HASH
value, and create a backup reflecting the edits.

A default limit of 50 backup files is kept, configurable by
lvm.conf devicesfile_backup_limit (set to 0 to disable backups.)

lib/commands/toolcontext.h

@@ -189,6 +189,8 @@ struct cmd_context {
 	unsigned pvscan_recreate_hints:1;	/* enable special case hint handling for pvscan --cache */
 	unsigned scan_lvs:1;
 	unsigned wipe_outdated_pvs:1;
+	unsigned devices_file_hash_mismatch:1;
+	unsigned devices_file_hash_ignore:1;
 	unsigned enable_devices_list:1;		/* command is using --devices option */
 	unsigned enable_devices_file:1;		/* command is using devices file */
 	unsigned pending_devices_file:1;	/* command may create and enable devices file */

lib/config/config_settings.h

@@ -292,6 +292,14 @@ cfg(devices_devicesfile_CFG, "devicesfile", devices_CFG_SECTION, CFG_DEFAULT_COM
 	"This should not be used to select a non-system devices file.\n"
 	"The --devicesfile option is intended for alternative devices files.\n")
 
+cfg(devices_devicesfile_backup_limit_CFG, "devicesfile_backup_limit", devices_CFG_SECTION, CFG_DEFAULT_COMMENTED, CFG_TYPE_INT, DEFAULT_DEVICESFILE_BACKUP_LIMIT, vsn(2, 3, 23), NULL, 0, NULL,
+	"The max number of backup files to keep in /etc/lvm/devices/backup.\n"
+	"LVM creates a backup of the devices file each time a new\n"
+	"version is created, or each time a modification is detected.\n"
+	"When the max number of backups is reached, the oldest are\n"
+	"removed to remain at the limit. Set to 0 to disable backups.\n"
+	"Only the system devices file is backed up.\n")
+
 cfg(devices_search_for_devnames_CFG, "search_for_devnames", devices_CFG_SECTION, CFG_DEFAULT_COMMENTED, CFG_TYPE_STRING, DEFAULT_SEARCH_FOR_DEVNAMES, vsn(2, 3, 12), NULL, 0, NULL,
 	"Look outside of the devices file for missing devname entries.\n"
 	"A devname entry is used for a device that does not have a stable\n"

lib/config/defaults.h

@@ -340,4 +340,6 @@
 
 #define DEFAULT_DEVICE_ID_SYSFS_DIR "/sys/"  /* trailing / to match dm_sysfs_dir() */
 
+#define DEFAULT_DEVICESFILE_BACKUP_LIMIT 50
+
 #endif				/* _LVM_DEFAULTS_H */

lib/device/device_id.c

@@ -30,6 +30,8 @@
 #include <fcntl.h>
 #include <unistd.h>
 #include <time.h>
+#include <dirent.h>
+#include <locale.h>
 #include <sys/types.h>
 #include <sys/file.h>
 #include <sys/sysmacros.h>
@@ -1167,6 +1169,9 @@ int device_ids_read(struct cmd_context *cmd)
 	char *idtype, *idname, *devname, *pvid, *part;
 	struct dev_use *du;
 	FILE *fp;
+	uint32_t comment_hash = 0;
+	uint32_t hash = INITIAL_CRC;
+	int ignore_hash = 0;
 	int line_error;
 	int product_uuid_found = 0;
 	int hostname_found = 0;
@@ -1197,6 +1202,24 @@ int device_ids_read(struct cmd_context *cmd)
 	}
 
 	while (fgets(line, sizeof(line), fp)) {
+
+		/* Special value for testing */
+		if (!strncmp(line, "# HASH=0", 8)) {
+			ignore_hash = 1;
+			continue;
+		}
+
+		if (!strncmp(line, "# HASH", 6)) {
+			_copy_idline_str(line, buf, sizeof(buf));
+			errno = 0;
+			comment_hash = (uint32_t)strtoul(buf, NULL, 10);
+			if (errno) {
+				log_debug("Devices file invalid hash value errno %d", errno);
+				comment_hash = 0;
+			}
+			continue;
+		}
+
 		if (line[0] == '#')
 			continue;
 
@@ -1204,6 +1227,8 @@ int device_ids_read(struct cmd_context *cmd)
 		if (!strncmp(line, "SYSTEMID", 8))
 			continue;
 
+		hash = calc_crc(hash, (uint8_t *)line, strlen(line));
+
 		if (!strncmp(line, "HOSTNAME", 8)) {
 			_copy_idline_str(line, check_id, sizeof(check_id));
 			log_debug("read devices file hostname %s", check_id);
@@ -1318,6 +1343,13 @@ int device_ids_read(struct cmd_context *cmd)
 	if (fclose(fp))
 		stack;
 
+	log_debug("Devices file comment hash %u calc hash %u", comment_hash, hash);
+
+	if (ignore_hash)
+		cmd->devices_file_hash_ignore = 1;
+	else if (hash != comment_hash)
+		cmd->devices_file_hash_mismatch = 1;
+
 	if (!product_uuid_found && cmd->device_ids_check_product_uuid) {
 		cmd->device_ids_refresh_trigger = 1;
 		log_debug("Devices file refresh: missing product_uuid");
@@ -1326,24 +1358,197 @@ int device_ids_read(struct cmd_context *cmd)
 		cmd->device_ids_refresh_trigger = 1;
 		log_debug("Devices file refresh: missing product_uuid and hostname");
 	}
-	
+
 	return ret;
 }
 
+#define BACKUP_NAME_LEN 35
+#define BACKUP_NAME_SIZE BACKUP_NAME_LEN+1 /* +1 null byte */
+
+static int _filter_backup_files(const struct dirent *de)
+{
+	if (strlen(de->d_name) != BACKUP_NAME_LEN)
+		return 0;
+	if (strncmp(de->d_name, "system.devices-", 15))
+		return 0;
+	return 1;
+}
+
+static void devices_file_backup(struct cmd_context *cmd, char *fc, char *fb, time_t *tp, uint32_t df_counter)
+{
+	struct dirent *de;
+	struct dirent **namelist;
+	DIR *dir;
+	FILE *fp;
+	struct tm *tm;
+	char dirpath[PATH_MAX];
+	char path[PATH_MAX];
+	char datetime_str[48];
+	char de_date_str[16];
+	char de_time_str[16];
+	char de_count_str[16];
+	char low_name[BACKUP_NAME_SIZE];
+	uint32_t low_date, low_time, low_count;
+	uint32_t de_date, de_time, de_count;
+	unsigned int backup_limit, backup_count, remove_count;
+	int sort_count;
+	int dir_fd;
+	int i;
+
+	/* Skip backup with --devicesfile <name>, only back up default system.devices. */
+	if (cmd->devicesfile)
+		return;
+	if (!(backup_limit = (unsigned int)find_config_tree_int(cmd, devices_devicesfile_backup_limit_CFG, NULL)))
+		return;
+	if (dm_snprintf(dirpath, sizeof(dirpath), "%s/devices/backup/", cmd->system_dir) < 0)
+		return;
+	if (!dm_create_dir(dirpath))
+		return;
+	if ((dir = opendir(dirpath)) < 0)
+		return;
+
+	tm = localtime(tp);
+	strftime(datetime_str, sizeof(datetime_str), "%Y%m%d.%H%M%S", tm);
+
+	/* system.devices-YYYYMMDD.HHMMSS.000N (fixed length 35) */
+	if (dm_snprintf(path, sizeof(path), "%s/devices/backup/system.devices-%s.%04u",
+			cmd->system_dir, datetime_str, df_counter) < 0)
+		return;
+
+	if (!(fp = fopen(path, "w+"))) {
+		log_error("Failed to create backup file %s", path);
+		return;
+	}
+	if (fputs(fc, fp) < 0) {
+		log_error("Failed to write backup file %s", path);
+		fclose(fp);
+		goto out;
+	}
+	if (fputs(fb, fp) < 0) {
+		log_error("Failed to write backup file %s", path);
+		fclose(fp);
+		goto out;
+	}
+	if (fflush(fp) < 0) {
+		log_error("Failed to write backup file %s", path);
+		fclose(fp);
+		goto out;
+	}
+	if (fsync(fileno(fp)) < 0) {
+		log_error("Failed to sync backup file %s", path);
+		fclose(fp);
+		goto out;
+	}
+	if (fclose(fp))
+		stack;
+
+	log_debug("Wrote backup %s", path);
+
+	/* Possibly remove old backup files per configurable limit on backup files. */
+
+	backup_count = 0;
+	low_date = 0;
+	low_time = 0;
+	low_count = 0;
+	memset(low_name, 0, sizeof(low_name)); /* oldest backup file name */
+
+	while ((de = readdir(dir))) {
+		if (de->d_name[0] == '.')
+			continue;
+		if (strlen(de->d_name) != BACKUP_NAME_LEN)
+			continue;
+
+		memset(de_date_str, 0, sizeof(de_date_str));
+		memset(de_time_str, 0, sizeof(de_time_str));
+		memset(de_count_str, 0, sizeof(de_count_str));
+
+		/*
+		 * Save the oldest backup file name.
+ 		 * system.devices-YYYYMMDD.HHMMSS.NNNN
+		 * 12345678901234567890123456789012345 (len 35)
+		 * date YYYYMMDD is 8 chars 16-23
+		 * time HHMMSS is 6 chars 25-30
+		 * count NNNN is 4 chars 32-35
+		 */
+		memcpy(de_date_str, de->d_name+15, 8);
+		memcpy(de_time_str, de->d_name+24, 6);
+		memcpy(de_count_str, de->d_name+31, 4);
+
+		de_date = (uint32_t)strtoul(de_date_str, NULL, 10);
+		de_time = (uint32_t)strtoul(de_time_str, NULL, 10);
+		de_count = (uint32_t)strtoul(de_count_str, NULL, 10);
+
+		if (!low_date ||
+		    (de_date < low_date) ||
+		    (de_date == low_date && de_time < low_time) ||
+		    (de_date == low_date && de_time == low_time && de_count < low_count)) {
+			strncpy(low_name, de->d_name, sizeof(low_name));
+			low_date = de_date;
+			low_time = de_time;
+			low_count = de_count;
+		}
+		backup_count++;
+	}
+
+	if (backup_count <= backup_limit)
+		goto out;
+
+	remove_count = backup_count - backup_limit;
+
+	dir_fd = dirfd(dir);
+
+	/* The common case removes the oldest file and can avoid sorting. */
+	if (remove_count == 1 && low_name[0]) {
+		log_debug("Remove backup %s", low_name);
+		unlinkat(dir_fd, low_name, 0);
+		goto out;
+	}
+
+	/* Remove the n oldest files by sorting system.devices-*. */
+	setlocale(LC_COLLATE, "C"); /* Avoid sorting by locales */
+	sort_count = scandir(dirpath, &namelist, _filter_backup_files, alphasort);
+	setlocale(LC_COLLATE, "");
+	if (sort_count < 0) {
+		log_error("Failed to sort backup devices files.");
+		goto out;
+	}
+
+	log_debug("Limit backup %u found %u sorted %d removing %u.",
+		  backup_limit, backup_count, sort_count, remove_count);
+
+	for (i = 0; i < sort_count; i++) {
+		if (remove_count) {
+			log_debug("Remove backup %s", namelist[i]->d_name);
+			unlinkat(dir_fd, namelist[i]->d_name, 0);
+			remove_count--;
+		}
+		free(namelist[i]);
+	}
+	free(namelist);
+out:
+	closedir(dir);
+}
+
 int device_ids_write(struct cmd_context *cmd)
 {
 	char dirpath[PATH_MAX];
 	char tmppath[PATH_MAX];
 	char version_buf[VERSION_LINE_MAX] = {0};
-	FILE *fp;
-	int dir_fd;
+	char fc[1024];		/* devices file comments (buf of commented lines) */
+	char *fb = NULL;	/* devices file contents (buf of uncommented lines) */
+	FILE *fp = NULL;
+	int dir_fd = -1;
 	time_t t;
 	struct dev_use *du;
 	const char *devname;
 	const char *pvid;
 	uint32_t df_major = 0, df_minor = 0, df_counter = 0;
+	uint32_t hash = 0;
+	int names_len = 0;
+	int len, num, pos;
+	int fb_size, fb_bytes, fc_bytes;
 	int file_exists;
-	int ret = 1;
+	int ret = 0;
 
 	if (!cmd->enable_devices_file && !cmd->pending_devices_file)
 		return 1;
@@ -1385,6 +1590,14 @@ int device_ids_write(struct cmd_context *cmd)
 		cmd->enable_devices_file = 1;
 	}
 
+	/* Total length of all devnames and idnames, used to estimate file size. */
+	dm_list_iterate_items(du, &cmd->use_devices) {
+		if (du->devname)
+			names_len += strlen(du->devname);
+		if (du->idname)
+			names_len += strlen(du->idname);
+	}
+
 	if (test_mode())
 		return 1;
 
@@ -1401,46 +1614,59 @@ int device_ids_write(struct cmd_context *cmd)
 		}
 	}
 
-	if (dm_snprintf(dirpath, sizeof(dirpath), "%s/devices", cmd->system_dir) < 0) {
-		ret = 0;
-		goto out;
-	}
+	if (dm_snprintf(dirpath, sizeof(dirpath), "%s/devices", cmd->system_dir) < 0)
+		goto_out;
 
-	if (dm_snprintf(tmppath, sizeof(tmppath), "%s_new", cmd->devices_file_path) < 0) {
-		ret = 0;
-		goto out;
-	}
+	if (dm_snprintf(tmppath, sizeof(tmppath), "%s_new", cmd->devices_file_path) < 0)
+		goto_out;
 
 	(void) unlink(tmppath); /* in case a previous file was left */
 
 	if (!(fp = fopen(tmppath, "w+"))) {
-		log_warn("Cannot open tmp devices_file to write.");
-		ret = 0;
+		log_warn("Cannot open to write %s.", tmppath);
 		goto out;
 	}
 
-	if ((dir_fd = open(dirpath, O_RDONLY)) < 0) {
-		if (fclose(fp))
-                        log_sys_debug("fclose", tmppath);
-		ret = 0;
+	if ((dir_fd = open(dirpath, O_RDONLY)) < 0)
+		goto_out;
+
+	/*
+	 * Estimate the size of the new system.devices:
+	 * names_len is the length of all devnames and idnames,
+	 * 256 bytes for PRODUCT_UUID/HOSTNAME and VERSION lines,
+	 * 128 bytes per device entry for other fields.
+	 */
+	fb_size = names_len + 256 + (128 * dm_list_size(&cmd->use_devices));
+
+	if (!(fb = malloc(fb_size))) {
+		log_error("Failed to allocate buffer size %d for devices file.", fb_size);
 		goto out;
 	}
 
-	t = time(NULL);
-
-	fprintf(fp, "# LVM uses devices listed in this file.\n");
-	fprintf(fp, "# Created by LVM command %s pid %d at %s", cmd->name, getpid(), ctime(&t));
+	len = fb_size;
+	pos = 0;
 
 	/* if product_uuid is included, then hostname is unnecessary */
 	if (cmd->product_uuid && cmd->device_ids_check_product_uuid)
-		fprintf(fp, "PRODUCT_UUID=%s\n", cmd->product_uuid);
+		num = snprintf(fb + pos, len - pos, "PRODUCT_UUID=%s\n", cmd->product_uuid);
 	else if (cmd->hostname && cmd->device_ids_check_hostname)
-		fprintf(fp, "HOSTNAME=%s\n", cmd->hostname);
+		num = snprintf(fb + pos, len - pos, "HOSTNAME=%s\n", cmd->hostname);
+
+	if (num >= len - pos) {
+		log_error("Failed to write buffer for devices file content.");
+		goto out;
+	}
+	pos += num;
 
 	if (dm_snprintf(version_buf, VERSION_LINE_MAX, "VERSION=%u.%u.%u", DEVICES_FILE_MAJOR, DEVICES_FILE_MINOR, df_counter+1) < 0)
-		stack;
-	else
-		fprintf(fp, "%s\n", version_buf);
+		goto_out;
+
+	num = snprintf(fb + pos, len - pos, "%s\n", version_buf);
+	if (num >= len - pos) {
+		log_error("Failed to write buffer for devices file content.");
+		goto out;
+	}
+	pos += num;
 
 	/* as if we had read this version in case we want to write again */
 	memset(_devices_file_version, 0, sizeof(_devices_file_version));
@@ -1457,33 +1683,83 @@ int device_ids_write(struct cmd_context *cmd)
 			pvid = du->pvid;
 
 		if (du->part) {
-			fprintf(fp, "IDTYPE=%s IDNAME=%s DEVNAME=%s PVID=%s PART=%d\n",
-				idtype_to_str(du->idtype) ?: ".",
-				du->idname ?: ".", devname, pvid, du->part);
+			num = snprintf(fb + pos, len - pos, "IDTYPE=%s IDNAME=%s DEVNAME=%s PVID=%s PART=%d\n",
+					idtype_to_str(du->idtype) ?: ".",
+					du->idname ?: ".", devname, pvid, du->part);
 		} else {
-			fprintf(fp, "IDTYPE=%s IDNAME=%s DEVNAME=%s PVID=%s\n",
-				idtype_to_str(du->idtype) ?: ".",
-				du->idname ?: ".", devname, pvid);
+			num = snprintf(fb + pos, len - pos, "IDTYPE=%s IDNAME=%s DEVNAME=%s PVID=%s\n",
+					idtype_to_str(du->idtype) ?: ".",
+					du->idname ?: ".", devname, pvid);
 		}
+		if (num >= len - pos) {
+			log_error("Failed to write buffer for devices file content.");
+			goto out;
+		}
+		pos += num;
 	}
+	fb_bytes = pos;
+	fb[fb_bytes] = '\0';
 
-	if (fflush(fp))
-		stack;
-	if (fclose(fp))
-		stack;
+	if (!cmd->devices_file_hash_ignore)
+		hash = calc_crc(INITIAL_CRC, (const uint8_t *)fb, fb_bytes);
+
+	t = time(NULL);
+
+	num = snprintf(fc, sizeof(fc),
+			"# LVM uses devices listed in this file.\n" \
+			"# Created by LVM command %s pid %d at %s" \
+			"# HASH=%u\n",
+			cmd->name, getpid(), ctime(&t), hash);
+
+	if (num >= sizeof(fc)) {
+		log_error("Failed to write buffer for devices file content.");
+		goto out;
+	}
+	fc_bytes = num;
+	fc[fc_bytes] = '\0';
+
+	if (fputs(fc, fp) < 0) {
+		log_error("Failed to write devices file header.");
+		goto out;
+	}
+	if (fputs(fb, fp) < 0) {
+		log_error("Failed to write devices file.");
+		goto out;
+	}
+	if (fflush(fp) < 0)
+		goto_out;
+	if (fsync(fileno(fp)) < 0)
+		goto_out;
+	if (fclose(fp) < 0)
+		goto_out;
+
+	fp = NULL;
 
 	if (rename(tmppath, cmd->devices_file_path) < 0) {
-		log_error("Failed to replace devices file errno %d", errno);
-		ret = 0;
+		log_error("Failed to replace devices file.");
+		goto out;
 	}
 
 	if (fsync(dir_fd) < 0)
 		stack;
 	if (close(dir_fd) < 0)
 		stack;
+	dir_fd = -1;
+
+	ret = 1;
+
+	log_debug("Wrote devices file %s hash %u hashed size %u total size %u",
+		  version_buf, hash, fb_bytes, fb_bytes + fc_bytes);
+
+	devices_file_backup(cmd, fc, fb, &t, df_counter+1);
 
-	log_debug("Wrote devices file %s", version_buf);
 out:
+	if (fb)
+		free(fb);
+	if (fp)
+		fclose(fp);
+	if (dir_fd > 0)
+		close(dir_fd);
 	return ret;
 }
 
@@ -3005,14 +3281,29 @@ void device_ids_validate(struct cmd_context *cmd, struct dm_list *scanned_devs,
 	/* FIXME: for wrong devname cases, wait to write new until device_ids_search? */
 
 	/*
-	 * try lock and device_ids_write(), the update is not required and will
-	 * be done by a subsequent command if it's not done here.
+	 * If an update is needed and allowed, then try lock and
+	 * device_ids_write(). The update is not required and will be done by a
+	 * subsequent command if it's not done here.
 	 */
-	if (update_file && noupdate) {
-		log_debug("Validated device ids: invalid=%d, update disabled.", cmd->device_ids_invalid);
-	} else if (update_file) {
-		log_debug("Validated device ids: invalid=%d, trying to update devices file.", cmd->device_ids_invalid);
-		_device_ids_update_try(cmd);
+
+	if (update_file) {
+		if (noupdate)
+			log_debug("Validated device ids: invalid=%d, update disabled.", cmd->device_ids_invalid);
+		else {
+			log_debug("Validated device ids: invalid=%d, trying to update devices file.", cmd->device_ids_invalid);
+			_device_ids_update_try(cmd);
+		}
+	} else if (cmd->devices_file_hash_mismatch) {
+		/*
+		 * The file was edited externally since lvm last wrote it, so the hash should be
+		 * updated and the file backed up.
+		 */
+		if (noupdate)
+			log_debug("Validated device ids: hash mismatch, update disabled.");
+		else {
+			log_debug("Validated device ids: hash mismatch, trying to update devices file.");
+			_device_ids_update_try(cmd);
+		}
 	} else {
 		log_debug("Validated device ids: invalid=%d, no update needed.", cmd->device_ids_invalid);
 	}

test/shell/devicesfile-backup.sh

@@ -0,0 +1,109 @@
+#!/usr/bin/env bash
+
+# Copyright (C) 2020 Red Hat, Inc. All rights reserved.
+#
+# This copyrighted material is made available to anyone wishing to use,
+# modify, copy, or redistribute it subject to the terms and conditions
+# of the GNU General Public License v.2.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software Foundation,
+# Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+test_description='devices file backups'
+
+SKIP_WITH_LVMPOLLD=1
+
+. lib/inittest
+
+aux prepare_devs 3
+
+aux lvmconf 'devices/use_devicesfile = 1'
+
+# Stupid tests use plain /etc/ rather than /etc/lvm/
+DFDIR="$LVM_SYSTEM_DIR/devices"
+BKDIR="$LVM_SYSTEM_DIR/devices/backup"
+mkdir -p "$DFDIR" || true
+mkdir -p "$BKDIR" || true
+DF="$DFDIR/system.devices"
+
+vgcreate $vg "$dev1"
+diff $DF $BKDIR/*.0001
+
+pvcreate "$dev2"
+diff $DF $BKDIR/*.0002
+
+lvmdevices --deldev "$dev2"
+diff $DF $BKDIR/*.0003
+
+lvmdevices --adddev "$dev2"
+diff $DF $BKDIR/*.0004
+
+# DF update and backup when an entry is manually removed
+cat $DF | grep -v "$dev2" > tmp1
+cp tmp1 $DF
+pvs
+diff $DF $BKDIR/*.0005
+
+lvmdevices --adddev "$dev2"
+diff $DF $BKDIR/*.0006
+
+# DF update and abckup when HASH value changes
+sed -e "s|HASH=.......|HASH=1111111|" $DF > tmp1
+cp tmp1 $DF
+pvs
+not grep "HASH=1111111" $DF
+diff $DF $BKDIR/*.0007
+
+# DF update and backup when old DF has no HASH value
+cat $DF | grep -v HASH > tmp1
+cp tmp1 $DF
+pvs
+grep HASH $DF
+diff $DF $BKDIR/*.0008
+
+# DF update and backup when dev names change
+pvcreate "$dev3"
+diff $DF $BKDIR/*.0009
+grep "$dev2" $DF
+grep "$dev3" $DF
+dd if="$dev2" of=dev2_header bs=1M count=1
+dd if="$dev3" of=dev3_header bs=1M count=1
+dd if=dev2_header of="$dev3"
+dd if=dev3_header of="$dev2"
+pvs
+diff $DF $BKDIR/*.0010
+
+# backup limit, remove 1
+aux lvmconf 'devices/devicesfile_backup_limit = 10'
+lvmdevices --deldev "$dev2"
+diff $DF $BKDIR/*.0011
+not ls $BKDIR/*.0001
+
+# backup limit, remove N
+aux lvmconf 'devices/devicesfile_backup_limit = 5'
+lvmdevices --adddev "$dev2"
+diff $DF $BKDIR/*.0012
+not ls $BKDIR/*.0002
+not ls $BKDIR/*.0003
+not ls $BKDIR/*.0004
+not ls $BKDIR/*.0005
+not ls $BKDIR/*.0006
+not ls $BKDIR/*.0007
+ls $BKDIR/*.0008
+ls $BKDIR/*.0009
+ls $BKDIR/*.0010
+ls $BKDIR/*.0011
+
+# backup disabled
+aux lvmconf 'devices/devicesfile_backup_limit = 0'
+lvmdevices --deldev "$dev2"
+not ls $BKDIR/*.0013
+
+# backup re-enabled
+aux lvmconf 'devices/devicesfile_backup_limit = 5'
+lvmdevices --adddev "$dev2"
+ls $BKDIR/*.0014
+not ls $BKDIR/*.0013
+
+vgremove -ff $vg

tools/lvmdevices.c

@@ -749,7 +749,7 @@ int lvmdevices(struct cmd_context *cmd, int argc, char **argv)
 		}
 
 		if (arg_is_set(cmd, update_ARG)) {
-			if (update_needed || !dm_list_empty(&found_devs)) {
+			if (update_needed || !dm_list_empty(&found_devs) || cmd->devices_file_hash_mismatch) {
 				if (!device_ids_write(cmd))
 					goto_bad;
 				log_print("Updated devices file to version %s", devices_file_version());
@@ -760,12 +760,22 @@ int lvmdevices(struct cmd_context *cmd, int argc, char **argv)
 			/*
 			 * --check exits with an error if the devices file
 			 * needs updates, i.e. running --update would make
-			 * changes.
+			 * changes to the devices entries.
 			 */
 			if (update_needed) {
 				log_error("Updates needed for devices file.");
 				goto bad;
 			}
+
+			/*
+			 * If only the hash comment would be updated, it isn't
+			 * considered a "real" update for purposes of the
+			 * --check exit code, since no device entries would be
+			 * changed (although --update would lead to a new
+			 * file version with the updated hash comment.)
+			 */
+			if (cmd->devices_file_hash_mismatch)
+				log_print("Hash update needed for devices file.");
 		}
 		goto out;
 	}