[PATCH 04/10] Explicitly parse version strings read from flash
Cyril Bur
cyrilbur at gmail.com
Tue Aug 29 16:06:58 AEST 2017
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 at 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);
> }
> }
>
More information about the Petitboot
mailing list