On Sun, Nov 26, 2017 at 02:38:18PM +0100, Marcin Wojtas wrote:
> Hi Leif,
>
> 2017-11-25 15:09 GMT+01:00 Leif Lindholm <leif.lindh...@linaro.org>:
> > > + // Check the Variable Store Guid
> > > + if (!CompareGuid (&VariableStoreHeader->Signature, &gEfiVariableGuid)
> > > &&
> > > + !CompareGuid (&VariableStoreHeader->Signature,
> > > + &gEfiAuthenticatedVariableGuid)) {
> >
> > Still spurious indentation. (If it's meant to indicate continuation,
> > that's fine, but that's two spaces, not three. Although I would find
> > !CompareGuid (&VariableStoreHeader->Signature,
> > &gEfiAuthenticatedVariableGuid)) {
> > more clear.
>
> It's two spaces from the beginning of function name. Anyway, I'll
> align &gEfiAuthenticatedVariableGuid to &VariableStoreHeader,
Thanks.
> > > +/**
> > > + The SetAttributes() function sets configurable firmware volume
> > > attributes
> > > + and returns the new settings of the firmware volume.
> > > +
> > > +
> > > + @param This EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL
> > > instance.
> > > +
> > > + @param Attributes On input, Attributes is a pointer to
> > > + EFI_FVB_ATTRIBUTES_2 that contains the
> > > desired
> > > + firmware volume settings.
> > > + On successful return, it contains the
> > > new
> > > + settings of the firmware volume.
> > > +
> > > + @retval EFI_SUCCESS The firmware volume attributes were
> > > returned.
> > > +
> > > + @retval EFI_INVALID_PARAMETER The attributes requested are in
> > > conflict with
> > > + the capabilities as declared in the
> > > firmware
> > > + volume header.
> > > +
> > > + **/
> > > +EFI_STATUS
> > > +EFIAPI
> > > +MvFvbSetAttributes (
> > > + IN CONST EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL *This,
> > > + IN OUT EFI_FVB_ATTRIBUTES_2 *Attributes
> > > + )
> > > +{
> > > + DEBUG ((DEBUG_BLKIO,
> > > + "%a: Operation not supported, keep default set of attributes\n",
> > > + __FUNCTION__));
> > > +
> > > + return MvFvbGetAttributes (This, Attributes);
> >
> > I'm still not completely thrilled about this.
> > Sure, the return value is always SUCCESS now.
> >
> > But the implicit meaning of the specification is that
> > *this*support*is*not*optional*.
>
> Ok, I'll change it then. I only referred to existing edk2-platforms
> implementations (AMD - 'return EFI_SUCCESS', Hisilicon/Socionext -
> 'return EFI_UNSUPPORTED' - all with doing nothing with the actual
> attributes list). Same with ArmPlatformPkg, only Intel seems to
> implement this.
Much appreciated.
Well, this is basically a case of reviewer Dunning-Kruger - when I
have previously reviewed such patches, I've not had sufficiently
internalised knowledge of the underlying protocols to go "hang on...".
I would appreciate if you could raise bugs on the items you have
discovered in ofther drivers in https://bugzilla.tianocore.org/.
/
Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
