Dear Takahiro,
In message 20190719082504.GS21948@linaro.org you wrote:
- if (ctx == ENVCTX_UEFI)
return do_env_print_efi(cmdtp, flag, argc, argv);I don't like this. It doesn't scale. ENVCTX_UEFI is just one context among others; it should not need a "if" here to handle.
Unfortunately, this is necessary. As you know, we want to allow different implementations of UEFI variables while we want to use "env" commands in the same way. do_env_print_efi(), for example, is implemented using *UEFI interfaces*, neither env_*() nor h*_r().
It is certainly not necessary to add conditional code here for each and every potential context. It should be able to provide a generic function, and each context can either use the default code, or implement his own. Then all you have to do here is to call the context's specific function pointer. No need for conditional code.
+#if defined(CONFIG_CMD_NVEDIT_EFI)
- if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e')
Please use proper argument handling, options can be combined, so the 'e' might not be the second character.
I think there are bunch of code like this in U-Boot. No?
Maybe, but this does not make it better / correct.
Also, this should probably changed to support a generic "context" specific handling, though "-c context" or such, with U-Boot being the default.
Yes, but please note that this option, -e, is already merged in the upstream.
This can be fixed, then?
This again allows to get rid of all these "if"s
I agree, but only when yet another context be introduced.
No, we should do it right from the beginning. It is not friendly to implement quick and dirty code and let the next who wants to use it do the cleanup.
Best regards,
Wolfgang Denk