Hi Manorit, On Wed, 11 Oct 2023 at 22:46, Manorit Chawdhry <m-chawd...@ti.com> wrote: > > Hi Simon, > > On 20:41-20231011, Simon Glass wrote: > > Hi Manorit, > > > > On Tue, 10 Oct 2023 at 23:25, Manorit Chawdhry <m-chawd...@ti.com> wrote: > > > > > > We can now firewall entities while loading them through our secure > > > entity TIFS, the required information should be present in the > > > certificate that is being parsed by TIFS. > > > > > > The following commit adds the support to enable the certificates to be > > > generated if the firewall configurations are present in the binman dtsi > > > nodes. > > > > > > Signed-off-by: Manorit Chawdhry <m-chawd...@ti.com> > > > --- > > > tools/binman/btool/openssl.py | 16 +++++++- > > > tools/binman/etype/ti_secure.py | 90 > > > +++++++++++++++++++++++++++++++++++++++++ > > > tools/binman/etype/x509_cert.py | 3 +- > > > 3 files changed, 106 insertions(+), 3 deletions(-) > > > > > > > Reviewed-by: Simon Glass <s...@chromium.org> > > > > I'm still a little worried about the error reporting if the user > > leaves out a property. Does it do the right thing? > > > > I did make a test also along the lines that would check all the > firewalling properties and just auth-in-place isn't checked at this > stage and people could end up adding the firewalling node without > auth-in-place. Do you want to cover that test case as well? Regarding > the other firewalling properties ( immitating the test by removing > "id"), We get this: > > [..] > AR spl/common/spl/built-in.o > LD spl/u-boot-spl > OBJCOPY spl/u-boot-spl-nodtb.bin > SYM spl/u-boot-spl.sym > CAT spl/u-boot-spl-dtb.bin > COPY spl/u-boot-spl.bin > BINMAN .binman_stamp > binman: id can't be None in firewall node
That's a bit vague...at least it should show the full path to the node that failure, e.g. use self.Raise(). Also, I assume the property is missing and the None refers to the Python value for the var. Better to provide a nice error. Any possible user error should ideally be checked, to avoid later confusion. See Entry.ensure_props() You could add an optional argument - the list of props to check. If present it could use that instead of self.required_props Hmm better might be to have a new function which tells the user that since the firewall property is present, it needs these others ones too. Regards, Simon