[Skiboot] [PATCH] pflash option to retrieve PNOR partition flags

[Patrick Williams]

On Thu, Jun 01, 2017 at 10:14:44PM -0500, Michael Tritz wrote:
> 
> Resolves openbmc/openbmc#1351

I don't think we should be resolving openbmc commits via skiboot
changes.  You'll end up needing to bump the skiboot reference in
order to incorporate these changes into openbmc anyhow, so that
version bump commit should have the Resolves.

> -		rc = ffs_part_info(ffs_handle, i, &name, &start, &size, &act, &ecc);
> +		rc = ffs_part_info(ffs_handle, i, &name, &start, &size, &act, NULL);

Is this ecc parsing still considered valid?  It doesn't look like you
explicitly removed support for the ecc parameter to ffs_part_info but
it doesn't look like there are really many users of it left either.

> +
> +		rc = ffs_part_flags(ffs_handle, i, &ecc, &preserved, &readonly,
> +				&backup, &reprovision);

Rather than another function call and a large number of variables passed
in here, should we make a ffs_flags struct with these booleans and
convert ffs_part_info to take an optional ffs_flags* instead of a bool*?

> +		if (rc == FFS_ERR_PART_NOT_FOUND){
> +			return;
> +		}
> +		if (rc) {
> +			fprintf(stderr, "Error %d scanning partitions\n", rc);
> +			return;
> +		}

These aren't possible at this point since you already made that same
check and lookup immediately prior when making the ffs_part_info call.
Another reason to me to combine them together into a single call.

> +		l = snprintf(flags, sizeof(flags), "[%s%s%s%s%s]",
> +				(ecc) ? "E" : "",
> +				(preserved) ? "P" : "",
> +				(readonly) ? "R" : "",
> +				(backup) ? "B" : "",
> +				(reprovision) ? "F" : "");

To bike-shed on this a bit, should you maybe print spaces for
'flag-not-set'?  You would end up with consistently displayed
width fields like "[E RB ]", "[EP  F]" that way.

(I believe the Hostboot team is going to be proposing a new 'Volatile'
flag in the near future.)

> +	rc = ffs_part_flags(ffs_handle, part_ID, &ecc, &preserved, &readonly,
> +			&backup, &reprovision);
> +	if (rc == FFS_ERR_PART_NOT_FOUND){
> +		fprintf(stderr, "Partition not found! Can't read flags.\n");
> +		return;
> +	}
> +	if (rc) {
> +		fprintf(stderr, "Error %d scanning partitions\n", rc);
> +		return;
> +	}
> +
> +	ffs_close(ffs_handle);

Any concern with handle leaks on the error paths?

> +
> +	char buf [strlen("ECC,PRESERVED,READONLY,BACKUP,REPROVISION.")];
> +	int l;
> +	l = snprintf(buf, sizeof(buf), "%s%s%s%s%s",
> +				 (ecc) ? "ECC," : "",
> +				 (preserved) ? "PRESERVED," : "",
> +				 (readonly) ? "READONLY," : "",
> +				 (backup) ? "BACKUP," : "",
> +				 (reprovision) ? "REPROVISION." : "");

Wouldn't this print a slightly awkward string like "ECC,READONLY," in
some cases but "REPROVISION." in others?

> +bool has_flag(struct ffs_entry *ent, uint8_t FLAG)
> +{
> +	return ((ent->user.miscflags & FLAG) != 0);
> +}

I'm curious if there is a broader preference on individual 'has_foo'
functions or converting 'has_ecc' invocations to 'has_flag(..., ECC)'?

> diff --git a/libflash/test/test-miscprint.pnor b/libflash/test/test-miscprint.pnor
> new file mode 100644
> index 0000000000000000000000000000000000000000..8a84ebaf079cb0af235ecd8568c3c990456d7764
> GIT binary patch
> literal 3072
> zcmWG=3<_ajU|@ve1|ZGKz`zWo7+63AG6--CyjqZ0RDvu9Wi$Q<0w5b?4oEYQ2Actu
> zrViCVudg#8$TiqCD9qIbVI;^-2B`f^Kqi<Erx5BOtOj%e;`!_9- at XoVb#(Ff^NB>#
> zg=|06?;u&IGmsR5nGWaz#Pc6*`TQ`*H6X}8%rn^2-w&n{*?v%*;rG7)OdIL+>dh;E
> zIXOCehX$bN1%){*{DEnQfc**})#QN}=k{&_#q%f_4S~@R7!85Z5Eu=C(GVC7fzc2k
> GKLh~mR(fav
> 
> literal 0
> HcmV?d00001

Is there some better way to formulate this?  A binary blob makes it hard
to perform diffs on to create new test cases.  Worst case you could
commit the output of 'xxd -p' in the repo and reverse as part of the
make check with 'xxd -pr'.

-- 
Patrick Williams
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://lists.ozlabs.org/pipermail/skiboot/attachments/20170605/8690069f/attachment-0001.sig>