Hi Eugeniu,
On Wed, 22 May 2019 at 01:11, Eugeniu Rosca erosca@de.adit-jv.com wrote:
Hi Simon,
Thanks for the review. Would you please reply to my questions below?
On Tue, May 21, 2019 at 06:53:29PM -0600, Simon Glass wrote:
Hi Eugeniu,
On Fri, 17 May 2019 at 08:46, Eugeniu Rosca erosca@de.adit-jv.com wrote:
[1] https://android.googlesource.com/platform/bootable/recovery [2] https://source.android.com/devices/bootloader [3] https://patchwork.ozlabs.org/patch/746835/ ("[U-Boot,5/6] Initial support for the Android Bootloader flow")
As discussed, please add these docs somewhere in the U-Boot tree (can be in a separate patch)
Sure.
if (blk_get_device_by_str("mmc", argv[1], &desc) < 0)Error codes should be reported.
printf("Error: Failed to read from mmc %s:%s\n", argv[1], argv[2]);Add error code here.
Will address.
switch (bcb_cmd_get(argv[0])) {Why have a switch() here, when you could just put the below code in each function? Or put the call to this function from the main do_bcb_set() function.
s/do_bcb_set/do_bcb/ ?
Yes
The switch() is there to tell the user that he misused specifically {sub}command 'bcb X'. If we just throw CMD_RET_USAGE (which means printing full-blown help text) on _any_ kind of command misuse , we don't help the user _at all_, IMHO. I would consider being user-friendly as the higher and more important policy. However, if you prioritize the code size over user experience, then I am open to rewrite the function. Would you please clarify which policy takes precedence between the two?
I think code size in commands is not the major priority, although we do tend to try to keep it moderate.
Here I wasn't suggesting removing code. I was just suggesting that the error handling could be in each specific command's function. So take the code out of each case statement and put into the function that it relates to.
Or continue to use the generic error handler, but call it from the generic function. It seems like we have:
do_bcb() { switch (cmd) { case CMD_FRED do_bcb_fred(); ... } ... }
do_bcb_fred() { check_args(CMD_FRED); ... }
check_args(int cmd) { switch (cmd) { case CMD_FRED: print error } }
I mean, put 'print error' inside do_bcb_fred()
or, call check_args() from do_bcb()
str = strdup(argv[2]);It is OK to change the args if you like.
I will try getting rid of strdup.
if (bcb_is_misused(argc, argv))return CMD_RET_FAILURE;You should return CMD_RET_USAGE if there is a usage problem.
This again connects with my previous statement on the user-experience. I would like to tell the user where exactly he did the mistake in using the 'bcb' rather than throwing a full-blown help message.
Yes this is good. See above where I tried to explain what I mean.
if (bcb_field_get(argv[1], &field, &size))return CMD_RET_FAILURE;print_buffer((ulong)field - (ulong)&bcb, (void *)field, 1, size, 16);Please add newline before return
Fine.
if (!strncmp(argv[2], "=", sizeof("="))) {Think about code size:
if (*argv[2] == '=')
Checking a single character will not detect the difference between '=' and '=anything' So, I have to validate, in addition, that the NULL terminator is there. But I agree with the comment in general.
Yes, understood (I would calll this 'nul', BTW)
[..]
Regards, Simon