cov: clean up pvid and vgid usage

pvid and vgid are sometimes a null-terminated string, and
other times a 'struct id', and the two types were often
cast between each other.  When a struct id was cast to a char
pointer, the resulting string would not necessarily be null
terminated.  Casting a null-terminated string id to a
struct id is fine, but is still avoided when possible.

A struct id is:  int8_t uuid[ID_LEN]
A string id is:  char pvid[ID_LEN + 1]

A convention is introduced to help distinguish them:

- variables and struct fields named "pvid" or "vgid"
  should be null-terminated strings.

- variables and struct fields named "pv_id" or "vg_id"
  should be struct id's.

- examples:
  char pvid[ID_LEN + 1];
  char vgid[ID_LEN + 1];
  struct id pv_id;
  struct id vg_id;

Function names also attempt to follow this convention.

Avoid casting between the two types as much as possible,
with limited exceptions when known to be safe and clearly
commented.

Avoid using variations of strcpy and strcmp, and instead
use memcpy/memcmp with ID_LEN (with similar limited
exceptions possible.)

4 files changed, 62 insertions, 16 deletions

diff --git a/lib/format_text/archiver.c b/lib/format_text/archiver.c
index f1590b460..fae368784 100644
--- a/lib/format_text/archiver.c
+++ b/lib/format_text/archiver.c
@@ -413,7 +413,8 @@ int backup_restore_vg(struct cmd_context *cmd, struct volume_group *vg,
 				return 0;
 			}
 			pv->vg_name = vg->name;
-			pv->vgid = vg->id;
+			/* both are struct id */
+			memcpy(&pv->vg_id, &vg->id, sizeof(struct id));
 
 			if (!(new_pvl = dm_pool_zalloc(vg->vgmem, sizeof(*new_pvl)))) {
 				log_error("Failed to allocate PV list item for \"%s\".",
diff --git a/lib/format_text/format-text.c b/lib/format_text/format-text.c
index 0441342a1..a274ab543 100644
--- a/lib/format_text/format-text.c
+++ b/lib/format_text/format-text.c
@@ -1667,8 +1667,9 @@ static int _set_ext_flags(struct physical_volume *pv, struct lvmcache_info *info
 /* Only for orphans - FIXME That's not true any more */
 static int _text_pv_write(struct cmd_context *cmd, const struct format_type *fmt, struct physical_volume *pv)
 {
+	char pvid[ID_LEN + 1] __attribute__((aligned(8))) = { 0 };
+	char vgid[ID_LEN + 1] __attribute__((aligned(8))) = { 0 };
 	struct format_instance *fid = pv->fid;
-	const char *pvid = (const char *) (*pv->old_id.uuid ? &pv->old_id : &pv->id);
 	struct label *label;
 	struct lvmcache_info *info;
 	struct mda_context *mdac;
@@ -1676,10 +1677,18 @@ static int _text_pv_write(struct cmd_context *cmd, const struct format_type *fmt
 	struct _write_single_mda_baton baton;
 	unsigned mda_index;
 
+	if (is_orphan_vg(pv->vg_name))
+		memcpy(vgid, pv->vg_name, ID_LEN);
+	else if (pv->vg)
+		memcpy(vgid, &pv->vg->id.uuid, ID_LEN);
+
+	memcpy(pvid, &pv->id.uuid, ID_LEN);
+
 	/* Add a new cache entry with PV info or update existing one. */
-	if (!(info = lvmcache_add(cmd, fmt->labeller, (const char *) &pv->id,
+	if (!(info = lvmcache_add(cmd, fmt->labeller, pvid,
 				  pv->dev,  pv->label_sector, pv->vg_name,
-				  is_orphan_vg(pv->vg_name) ? pv->vg_name : pv->vg ? (const char *) &pv->vg->id : NULL, 0, NULL)))
+				  vgid[0] ? vgid : NULL,
+				  0, NULL)))
 		return_0;
 
 	/* lvmcache_add() creates info and info->label structs for the dev, get info->label. */
@@ -1697,6 +1706,13 @@ static int _text_pv_write(struct cmd_context *cmd, const struct format_type *fmt
 	 * The fid_get_mda_indexed fn can handle that transparently,
 	 * just pass the right format_instance in.
 	 */
+
+	/* FIXME: why is old needed here? */
+	if (*pv->old_id.uuid)
+		memcpy(pvid, &pv->old_id.uuid, ID_LEN);
+	else
+		memcpy(pvid, &pv->id.uuid, ID_LEN);
+
 	for (mda_index = 0; mda_index < FMT_TEXT_MAX_MDAS_PER_PV; mda_index++) {
 		if (!(mda = fid_get_mda_indexed(fid, pvid, ID_LEN, mda_index)))
 			continue;
@@ -1775,7 +1791,7 @@ static int _text_pv_needs_rewrite(const struct format_type *fmt, struct physical
 	if (!pv->dev)
 		return 1;
 
-	if (!(info = lvmcache_info_from_pvid((const char *)&pv->id, pv->dev, 0))) {
+	if (!(info = lvmcache_info_from_pv_id(&pv->id, pv->dev, 0))) {
 		log_error("Failed to find cached info for PV %s.", pv_dev_name(pv));
 		return 0;
 	}
@@ -2007,8 +2023,8 @@ static int _text_pv_setup(const struct format_type *fmt,
 			  struct physical_volume *pv,
 			  struct volume_group *vg)
 {
+	char pvid[ID_LEN + 1] __attribute__((aligned(8))) = { 0 };
 	struct format_instance *fid = pv->fid;
-	const char *pvid = (const char *) (*pv->old_id.uuid ? &pv->old_id : &pv->id);
 	struct lvmcache_info *info;
 	unsigned mda_index;
 	struct metadata_area *pv_mda, *pv_mda_copy;
@@ -2016,6 +2032,11 @@ static int _text_pv_setup(const struct format_type *fmt,
 	uint64_t pe_count;
 	uint64_t size_reduction = 0;
 
+	if (*pv->old_id.uuid)
+		memcpy(pvid, &pv->old_id.uuid, ID_LEN);
+	else
+		memcpy(pvid, &pv->id.uuid, ID_LEN);
+
 	/* If PV has its own format instance, add mdas from pv->fid to vg->fid. */
 	if (pv->fid != vg->fid) {
 		for (mda_index = 0; mda_index < FMT_TEXT_MAX_MDAS_PER_PV; mda_index++) {
@@ -2181,6 +2202,7 @@ static int _add_metadata_area_to_pv(struct physical_volume *pv,
 				    uint64_t mda_size,
 				    unsigned mda_ignored)
 {
+	char pvid[ID_LEN + 1] __attribute__((aligned(8))) = { 0 };
 	struct metadata_area *mda;
 	struct mda_context *mdac;
 	struct mda_lists *mda_lists = (struct mda_lists *) pv->fmt->private;
@@ -2215,7 +2237,9 @@ static int _add_metadata_area_to_pv(struct physical_volume *pv,
 	memset(&mdac->rlocn, 0, sizeof(mdac->rlocn));
 	mda_set_ignored(mda, mda_ignored);
 
-	fid_add_mda(pv->fid, mda, (char *) &pv->id, ID_LEN, mda_index);
+	memcpy(pvid, &pv->id.uuid, ID_LEN);
+
+	fid_add_mda(pv->fid, mda, pvid, ID_LEN, mda_index);
 
 	return 1;
 }
@@ -2231,8 +2255,8 @@ static int _text_pv_add_metadata_area(const struct format_type *fmt,
 				      uint64_t mda_size,
 				      unsigned mda_ignored)
 {
+	char pvid[ID_LEN + 1] __attribute__((aligned(8))) = { 0 };
 	struct format_instance *fid = pv->fid;
-	const char *pvid = (const char *) (*pv->old_id.uuid ? &pv->old_id : &pv->id);
 	uint64_t ba_size, pe_start, first_unallocated;
 	uint64_t alignment, alignment_offset;
 	uint64_t disk_size;
@@ -2246,6 +2270,11 @@ static int _text_pv_add_metadata_area(const struct format_type *fmt,
 	const char *limit_name;
 	int limit_applied = 0;
 
+	if (*pv->old_id.uuid)
+		memcpy(pvid, &pv->old_id.uuid, ID_LEN);
+	else
+		memcpy(pvid, &pv->id.uuid, ID_LEN);
+
 	if (mda_index >= FMT_TEXT_MAX_MDAS_PER_PV) {
 		log_error(INTERNAL_ERROR "invalid index of value %u used "
 			      "while trying to add metadata area on PV %s. "
@@ -2475,6 +2504,8 @@ bad:
 static int _remove_metadata_area_from_pv(struct physical_volume *pv,
 					 unsigned mda_index)
 {
+	char pvid[ID_LEN + 1] __attribute__((aligned(8))) = { 0 };
+
 	if (mda_index >= FMT_TEXT_MAX_MDAS_PER_PV) {
 		log_error(INTERNAL_ERROR "can't remove metadata area with "
 					 "index %u from PV %s. Metadata "
@@ -2484,8 +2515,9 @@ static int _remove_metadata_area_from_pv(struct physical_volume *pv,
 		return 0;
 	}
 
-	return fid_remove_mda(pv->fid, NULL, (const char *) &pv->id,
-			      ID_LEN, mda_index);
+	memcpy(pvid, &pv->id.uuid, ID_LEN);
+
+	return fid_remove_mda(pv->fid, NULL, pvid, ID_LEN, mda_index);
 }
 
 static int _text_pv_remove_metadata_area(const struct format_type *fmt,
@@ -2500,14 +2532,19 @@ static int _text_pv_resize(const struct format_type *fmt,
 			   struct volume_group *vg,
 			   uint64_t size)
 {
+	char pvid[ID_LEN + 1] __attribute__((aligned(8))) = { 0 };
 	struct format_instance *fid = pv->fid;
-	const char *pvid = (const char *) (*pv->old_id.uuid ? &pv->old_id : &pv->id);
 	struct metadata_area *mda;
 	struct mda_context *mdac;
 	uint64_t size_reduction;
 	uint64_t mda_size;
 	unsigned mda_ignored;
 
+	if (*pv->old_id.uuid)
+		memcpy(pvid, &pv->old_id.uuid, ID_LEN);
+	else
+		memcpy(pvid, &pv->id.uuid, ID_LEN);
+
 	/*
 	 * First, set the new size and update the cache and reset pe_count.
 	 * (pe_count must be reset otherwise it would be considered as
diff --git a/lib/format_text/import_vsn1.c b/lib/format_text/import_vsn1.c
index 946184b4b..dacf5573e 100644
--- a/lib/format_text/import_vsn1.c
+++ b/lib/format_text/import_vsn1.c
@@ -219,7 +219,8 @@ static int _read_pv(struct cmd_context *cmd,
 	if (!(pv->vg_name = dm_pool_strdup(mem, vg->name)))
 		return_0;
 
-	memcpy(&pv->vgid, &vg->id, sizeof(vg->id));
+	/* both are struct id */
+	memcpy(&pv->vg_id, &vg->id, sizeof(struct id));
 
 	if (!_read_flag_config(pvn, &pv->status, PV_FLAGS)) {
 		log_error("Couldn't read status flags for physical volume.");
@@ -1302,6 +1303,7 @@ static int _read_vgsummary(const struct format_type *fmt, const struct dm_config
 	const struct dm_config_node *vgn;
 	struct dm_pool *mem = fmt->cmd->mem;
 	const char *str;
+	struct id id;
 
 	if (!dm_config_get_str(cft->root, "creation_host", &str))
 		str = "";
@@ -1322,11 +1324,13 @@ static int _read_vgsummary(const struct format_type *fmt, const struct dm_config
 
 	vgn = vgn->child;
 
-	if (!_read_id(&vgsummary->vgid, vgn, "id")) {
+	if (!_read_id(&id, vgn, "id")) {
 		log_error("Couldn't read uuid for volume group %s.", vgsummary->vgname);
 		return 0;
 	}
 
+	memcpy(vgsummary->vgid, &id, ID_LEN);
+
 	if (!_read_flag_config(vgn, &vgsummary->vgstatus, VG_FLAGS)) {
 		log_error("Couldn't find status flags for volume group %s.",
 			  vgsummary->vgname);
diff --git a/lib/format_text/text_label.c b/lib/format_text/text_label.c
index 7bc7a1ed3..f12171eca 100644
--- a/lib/format_text/text_label.c
+++ b/lib/format_text/text_label.c
@@ -409,6 +409,7 @@ static int _text_read(struct cmd_context *cmd, struct labeller *labeller, struct
 		      uint64_t label_sector, int *is_duplicate)
 {
 	struct lvmcache_vgsummary vgsummary;
+	char pvid[ID_LEN + 1] __attribute__((aligned(8))) = { 0 };
 	struct lvmcache_info *info;
 	const struct format_type *fmt = labeller->fmt;
 	struct label_header *lh = (struct label_header *) label_buf;
@@ -431,6 +432,8 @@ static int _text_read(struct cmd_context *cmd, struct labeller *labeller, struct
 	 */
 	pvhdr = (struct pv_header *) ((char *) label_buf + xlate32(lh->offset_xl));
 
+	memcpy(pvid, &pvhdr->pv_uuid, ID_LEN);
+
 	/*
 	 * FIXME: stop adding the device to lvmcache initially as an orphan
 	 * (and then moving it later) and instead just add it when we know the
@@ -445,7 +448,7 @@ static int _text_read(struct cmd_context *cmd, struct labeller *labeller, struct
 	 *
 	 * Other reasons for lvmcache_add to return NULL are internal errors.
 	 */
-	if (!(info = lvmcache_add(cmd, labeller, (char *)pvhdr->pv_uuid, dev, label_sector,
+	if (!(info = lvmcache_add(cmd, labeller, pvid, dev, label_sector,
 				  FMT_TEXT_ORPHAN_VG_NAME,
 				  FMT_TEXT_ORPHAN_VG_NAME, 0, is_duplicate)))
 		return_0;
@@ -495,8 +498,9 @@ static int _text_read(struct cmd_context *cmd, struct labeller *labeller, struct
 	if (!(ext_version = xlate32(pvhdr_ext->version)))
 		goto scan_mdas;
 
-	log_debug_metadata("%s: PV header extension version " FMTu32 " found",
-			   dev_name(dev), ext_version);
+	if (ext_version != PV_HEADER_EXTENSION_VSN)
+		log_debug_metadata("Found pv_header_extension version " FMTu32 " on %s",
+				   ext_version, dev_name(dev));
 
 	/* Extension version */
 	lvmcache_set_ext_version(info, xlate32(pvhdr_ext->version));