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

[Cyril Bur]

On Mon, 2017-06-05 at 17:02 -0500, Patrick Williams wrote:
> 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*?
> 

Yes! If you look in libffs.h theres already ffs_entry_new()
ffs_entry_add(), looks like what should be added is an ffs_entry_get()
which could then be passed to an ffs_entry_user_get() - and it would
just need to do the inverse of ffs_entry_user_get()...

The return of ffs_entry_get() should remain opaque - the internals
could change.

> > +		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.
> 

Agreed!

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

I think I've already seen the flag lying around - probably unused for
now but yes, I suppose it could appear in pnor at any time.

> 
> > +	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?
> 

Also, I'm not sure I follow the purpose of this function - I like the
idea of showing the flags with --info and with a bit of grep reproduing
this output is trivial...

If we're going to do this, it would better to have a --detail or
something where pflash prints one specific partitions info in greater
detail, print everything, not just the flags.

> > +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'.
> 
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot