[04/10] Explicitly parse version strings read from flash

Currently the version strings obtained from lib/flash are used as-is for
display only. To usefully compare versions however this strings need to
be separated into at least package name and package version.

On the OpenPOWER platforms from which these versions are obtained there
is a known list of package names, which hostboot_load_versions() now
uses to parse each version string into a new firmware_version struct
containing the name and version strings.

Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
---
 discover/hostboot.c           |  85 +++++++++++++++++++++++++++++----
 lib/flash/flash.c             |   4 +-
 lib/pb-protocol/pb-protocol.c | 108 +++++++++++++++++++++++++++++++++---------
 lib/types/types.h             |  13 ++++-
 ui/ncurses/nc-sysinfo.c       |   6 ++-
 5 files changed, 180 insertions(+), 36 deletions(-)

On Fri, 2017-08-25 at 15:59 +1000, Samuel Mendoza-Jonas wrote:
> Currently the version strings obtained from lib/flash are used as-is for
> display only. To usefully compare versions however this strings need to
> be separated into at least package name and package version.
> 
> On the OpenPOWER platforms from which these versions are obtained there
> is a known list of package names, which hostboot_load_versions() now
> uses to parse each version string into a new firmware_version struct
> containing the name and version strings.
> 

I'm going to assume that the serialising/deserialising code works...
theres nothing obviously wrong but I don't look at that code enough to
spot problems.

Other comments inline.

> Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
> ---
>  discover/hostboot.c           |  85 +++++++++++++++++++++++++++++----
>  lib/flash/flash.c             |   4 +-
>  lib/pb-protocol/pb-protocol.c | 108 +++++++++++++++++++++++++++++++++---------
>  lib/types/types.h             |  13 ++++-
>  ui/ncurses/nc-sysinfo.c       |   6 ++-
>  5 files changed, 180 insertions(+), 36 deletions(-)
> 
> diff --git a/discover/hostboot.c b/discover/hostboot.c
> index bc9a3ba..c63e363 100644
> --- a/discover/hostboot.c
> +++ b/discover/hostboot.c
> @@ -8,19 +8,88 @@
>  
>  #include "hostboot.h"
>  
> +/* Check each version string against a list of known package names and split
> + * them into separate name and version strings.
> + * In a nicer world we would just split based upon a known delimeter but version
> + * strings on OpenPOWER platforms are not consistent.
> + */
> +static int parse_hostboot_versions(void *ctx, char **strings,
> +		unsigned int n_str, struct firmware_version **versions)
> +{
> +	const char * cmp_str[] = {"open-power", "buildroot", "skiboot",
> +		"hostboot-binaries", "hostboot", "linux", "petitboot", "occ",
> +		"capp-ucode", "machine-xml"};
> +	unsigned int n_names = sizeof(cmp_str) / sizeof(cmp_str[0]);
> +	struct firmware_version *tmp = NULL;
> +	unsigned int i, j, n_versions = 0;
> +	bool found;
> +	char *sep;
> +
> +	for (i = 0; i < n_str; i++) {
> +		found = false;
> +		for (j = 0; j < n_names; j++) {
> +			if (strncmp(strings[i], cmp_str[j],
> +						strlen(cmp_str[j])) == 0) {
> +				found = true;
> +				break;
> +			}
> +		}
> +		if (!found) {
> +			pb_log("Unrecognised version in flash: %s\n",
> +					strings[i]);
> +			continue;
> +		}
> +
> +		sep = strings[i] + strlen(cmp_str[j]);
> +		*sep = '\0';
> +		tmp = talloc_realloc(ctx, tmp, struct firmware_version,
> +				n_versions + 1);
> +		if (!tmp)
> +			return -ENOMEM;

The talloc manpage isn't super clear about what happens to the pointer
that was passed if talloc_realloc() fails. Doing this with standard
realloc() would leak the 'old' tmp, but this is is talloc, so, I guess
it's never quite leaked, although if ctx for the lifetime of petitboot
its effectively a leak. 

I assume you only call this once (ok twice because of sides) at startup
so it's not like this can cause a leak over time.

*shrugs*

> +		tmp[n_versions].name = talloc_strdup(tmp, strings[i]);
> +		tmp[n_versions].version = talloc_strdup(tmp, sep + 1);
> +		n_versions++;
> +	}
> +
> +	*versions = tmp;
> +	return n_versions;
> +}
> +
>  void hostboot_load_versions(struct system_info *info)
>  {
> +	char **strings = NULL;
>  	int n = 0;
>  
> -	n = flash_parse_version(info, &info->platform_primary, true);
> -	if (n < 0)
> +	n = flash_parse_version(info, &strings, true);
> +	if (n < 0) {
>  		pb_log("Failed to read platform versions for current side\n");
> -	else
> -		info->n_primary = n;
> +		return;
> +	}
>  
> -	n = flash_parse_version(info, &info->platform_other, false);
> -	if (n < 0)
> +	n = parse_hostboot_versions(info, strings, n, &info->platform_primary);
> +	if (n < 0) {
> +		pb_log("Failed to parse platform versions for current side\n");
> +		return;
> +	}
> +	info->n_primary = n;
> +
> +	n = 0;
> +	talloc_free(strings);
> +	strings = NULL;
> +
> +	n = flash_parse_version(info, &strings, false);
> +	if (n < 0) {
>  		pb_log("Failed to read platform versions for other side\n");
> -	else
> -		info->n_other = n;
> +		return;
> +	}
> +
> +	n = parse_hostboot_versions(info, strings, n, &info->platform_other);
> +	if (n < 0) {
> +		pb_log("Failed to parse platform versions for current side\n");
> +		return;
> +	}
> +	info->n_other = n;
> +
> +
> +	talloc_free(strings);
>  }
> diff --git a/lib/flash/flash.c b/lib/flash/flash.c
> index 1384118..1da7010 100644
> --- a/lib/flash/flash.c
> +++ b/lib/flash/flash.c
> @@ -190,7 +190,7 @@ int flash_parse_version(void *ctx, char ***versions, bool current)
>  			pb_log("%s: Failed to allocate memory\n", __func__);
>  			goto out;
>  		}
> -		tmp[n++] = talloc_strdup(ctx, tok);
> +		tmp[n++] = talloc_strdup(tmp, tok);
>  	}
>  
>  	tok = strtok_r(NULL, delim, &saveptr);
> @@ -202,7 +202,7 @@ int flash_parse_version(void *ctx, char ***versions, bool current)
>  			n = 0;
>  			goto out;
>  		}


Just adding extra context:

         tok = strtok_r(NULL, delim, &saveptr);
         while (tok) {    
                 /* Ignore leading tab from subsequent lines */
                 tmp = talloc_realloc(ctx, tmp, char *, n +
1);                                                                    
                                                         
                 if (!tmp) {
                         pb_log("%s: Failed to reallocate memory\n",
__func__);
                         n = 0;
                         goto out;
                 }        

> -		tmp[n++] = talloc_strdup(ctx, tok + 1);
> +		tmp[n++] = talloc_strdup(tmp, tok + 1);

I see what you're trying to do - so the caller can just
talloc_free(strings);. However, how do the internals of
talloc_realloc() work? will the talloc_strdup() functions which used
different tmp pointers in previous iterations of the loop still be
associated to the new tmp pointer?

I assume you tested this and it didn't blow up so presumably talloc did
something sensible - or there were a few leaks?

>  		tok = strtok_r(NULL, delim, &saveptr);
>  	}
>  
> diff --git a/lib/pb-protocol/pb-protocol.c b/lib/pb-protocol/pb-protocol.c
> index dbbda40..76a2665 100644
> --- a/lib/pb-protocol/pb-protocol.c
> +++ b/lib/pb-protocol/pb-protocol.c
> @@ -226,6 +226,21 @@ int pb_protocol_boot_status_len(const struct status *status)
>  		4;
>  }
>  
> +static int pb_protocol_firmware_version_len(
> +		const struct firmware_version *versions,
> +		unsigned int n_versions)
> +{
> +	unsigned int i;
> +	int len = 0;
> +
> +	for (i = 0; i < n_versions; i++) {
> +		len += 4 + optional_strlen(versions[i].name);
> +		len += 4 + optional_strlen(versions[i].version);
> +	}
> +
> +	return len;
> +}
> +
>  int pb_protocol_system_info_len(const struct system_info *sysinfo)
>  {
>  	unsigned int len, i;
> @@ -234,12 +249,15 @@ int pb_protocol_system_info_len(const struct system_info *sysinfo)
>  		4 + optional_strlen(sysinfo->identifier) +
>  		4 + 4;
>  
> -	len +=	4;
> -	for (i = 0; i < sysinfo->n_primary; i++)
> -		len += 4 + optional_strlen(sysinfo->platform_primary[i]);
> -	len +=	4;
> -	for (i = 0; i < sysinfo->n_other; i++)
> -		len += 4 + optional_strlen(sysinfo->platform_other[i]);
> +	len +=  4 /* n_primary */;
> +	len += pb_protocol_firmware_version_len(sysinfo->platform_primary,
> +			sysinfo->n_primary);
> +	len +=  4 /* n_other */;
> +	len += pb_protocol_firmware_version_len(sysinfo->platform_other,
> +			sysinfo->n_other);
> +	len +=  4 /* n_new */;
> +	len += pb_protocol_firmware_version_len(sysinfo->platform_new,
> +			sysinfo->n_new);
>  
>  	len +=	4;
>  	for (i = 0; i < sysinfo->n_bmc_current; i++)
> @@ -447,6 +465,21 @@ int pb_protocol_serialise_boot_status(const struct status *status,
>  	return 0;
>  }
>  
> +static int pb_protocol_serialse_firmware_version(
> +		const struct firmware_version *versions,
> +		unsigned int n_versions, char *buf)
> +{
> +	char *pos = buf;
> +	unsigned int i;
> +
> +	for (i = 0; i < n_versions; i++) {
> +		pos += pb_protocol_serialise_string(pos, versions[i].name);
> +		pos += pb_protocol_serialise_string(pos, versions[i].version);
> +	}
> +
> +	return pos - buf;
> +}
> +
>  int pb_protocol_serialise_system_info(const struct system_info *sysinfo,
>  		char *buf, int buf_len)
>  {
> @@ -458,13 +491,18 @@ int pb_protocol_serialise_system_info(const struct system_info *sysinfo,
>  
>  	*(uint32_t *)pos = __cpu_to_be32(sysinfo->n_primary);
>  	pos += sizeof(uint32_t);
> -	for (i = 0; i < sysinfo->n_primary; i++)
> -		pos += pb_protocol_serialise_string(pos, sysinfo->platform_primary[i]);
> +	pos += pb_protocol_serialse_firmware_version(
> +			sysinfo->platform_primary, sysinfo->n_primary, pos);
>  
>  	*(uint32_t *)pos = __cpu_to_be32(sysinfo->n_other);
>  	pos += sizeof(uint32_t);
> -	for (i = 0; i < sysinfo->n_other; i++)
> -		pos += pb_protocol_serialise_string(pos, sysinfo->platform_other[i]);
> +	pos += pb_protocol_serialse_firmware_version(
> +			sysinfo->platform_other, sysinfo->n_other, pos);
> +
> +	*(uint32_t *)pos = __cpu_to_be32(sysinfo->n_new);
> +	pos += sizeof(uint32_t);
> +	pos += pb_protocol_serialse_firmware_version(
> +			sysinfo->platform_new, sysinfo->n_new, pos);
>  
>  	*(uint32_t *)pos = __cpu_to_be32(sysinfo->n_bmc_current);
>  	pos += sizeof(uint32_t);
> @@ -921,6 +959,25 @@ out:
>  	return rc;
>  }
>  
> +static int pb_protocol_deserialise_firmware_version(const char **buf,
> +		unsigned int *len, struct firmware_version *versions,
> +		unsigned int n_versions)
> +{
> +	unsigned int i;
> +	char *tmp;
> +
> +	for (i = 0; i < n_versions; i++) {
> +		if (read_string(versions, buf, len, &tmp))
> +			return -1;
> +		versions[i].name = talloc_strdup(versions, tmp);
> +		if (read_string(versions, buf, len, &tmp))
> +			return -1;
> +		versions[i].version = talloc_strdup(versions, tmp);
> +	}
> +
> +	return 0;
> +}
> +
>  int pb_protocol_deserialise_system_info(struct system_info *sysinfo,
>  		const struct pb_protocol_message *message)
>  {
> @@ -942,23 +999,30 @@ int pb_protocol_deserialise_system_info(struct system_info *sysinfo,
>  	/* Platform version strings for openpower platforms */
>  	if (read_u32(&pos, &len, &sysinfo->n_primary))
>  		goto out;
> -	sysinfo->platform_primary = talloc_array(sysinfo, char *,
> +	sysinfo->platform_primary = talloc_array(sysinfo,
> +						struct firmware_version,
>  						sysinfo->n_primary);
> -	for (i = 0; i < sysinfo->n_primary; i++) {
> -		if (read_string(sysinfo, &pos, &len, &tmp))
> -			goto out;
> -		sysinfo->platform_primary[i] = talloc_strdup(sysinfo, tmp);
> -	}
> +	if (pb_protocol_deserialise_firmware_version(&pos, &len,
> +				sysinfo->platform_primary, sysinfo->n_primary))
> +		goto out;
>  
>  	if (read_u32(&pos, &len, &sysinfo->n_other))
>  		goto out;
> -	sysinfo->platform_other = talloc_array(sysinfo, char *,
> +	sysinfo->platform_other = talloc_array(sysinfo,
> +						struct firmware_version,
>  						sysinfo->n_other);
> -	for (i = 0; i < sysinfo->n_other; i++) {
> -		if (read_string(sysinfo, &pos, &len, &tmp))
> -			goto out;
> -		sysinfo->platform_other[i] = talloc_strdup(sysinfo, tmp);
> -	}
> +	if (pb_protocol_deserialise_firmware_version(&pos, &len,
> +				sysinfo->platform_other, sysinfo->n_other))
> +		goto out;
> +
> +	if (read_u32(&pos, &len, &sysinfo->n_new))
> +		goto out;
> +	sysinfo->platform_new = talloc_array(sysinfo,
> +						struct firmware_version,
> +						sysinfo->n_new);
> +	if (pb_protocol_deserialise_firmware_version(&pos, &len,
> +				sysinfo->platform_new, sysinfo->n_new))
> +		goto out;
>  
>  	/* BMC version strings for openpower platforms */
>  	if (read_u32(&pos, &len, &sysinfo->n_bmc_current))
> diff --git a/lib/types/types.h b/lib/types/types.h
> index 9ab2a43..f4312e6 100644
> --- a/lib/types/types.h
> +++ b/lib/types/types.h
> @@ -118,13 +118,22 @@ struct blockdev_info {
>  	char		*mountpoint;
>  };
>  
> +struct firmware_version {
> +	char		*name;
> +	char		*version;
> +};
> +
>  struct system_info {
>  	char			*type;
>  	char			*identifier;
> -	char			**platform_primary;
> -	char			**platform_other;
> +
> +	struct firmware_version *platform_primary;
>  	unsigned int		n_primary;
> +	struct firmware_version *platform_other;
>  	unsigned int		n_other;
> +	struct firmware_version *platform_new;
> +	unsigned int		n_new;
> +
>  	char			**bmc_current;
>  	char			**bmc_golden;
>  	unsigned int		n_bmc_current;
> diff --git a/ui/ncurses/nc-sysinfo.c b/ui/ncurses/nc-sysinfo.c
> index 8252b45..2d3ce82 100644
> --- a/ui/ncurses/nc-sysinfo.c
> +++ b/ui/ncurses/nc-sysinfo.c
> @@ -69,7 +69,8 @@ static void sysinfo_screen_populate(struct sysinfo_screen *screen,
>  		line(NULL);
>  		line("%s", _("Primary platform versions:"));
>  		for (i = 0; i < sysinfo->n_primary; i++) {
> -			line("\t%s", sysinfo->platform_primary[i] ?: "");
> +			line("\t%s: %s", sysinfo->platform_primary[i].name,
> +					sysinfo->platform_primary[i].version);
>  		}
>  	}
>  
> @@ -77,7 +78,8 @@ static void sysinfo_screen_populate(struct sysinfo_screen *screen,
>  		line(NULL);
>  		line("%s", _("Alternate platform versions:"));
>  		for (i = 0; i < sysinfo->n_other; i++) {
> -			line("\t%s", sysinfo->platform_other[i] ?: "");
> +			line("\t%s: %s", sysinfo->platform_other[i].name,
> +					sysinfo->platform_other[i].version);
>  		}
>  	}
>