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/ ?
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?
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.
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.
if (part_get_info(desc, bcb_part, &info))goto err;Again, error numbers need to be printed.
Will address.
Regards, Simon
Thanks!