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 <ero...@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! -- Best Regards, Eugeniu. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot