[U-Boot] [PATCH v3 1/2] Loop block device for sandbox

Marek Vasut
2012-09-05T12:52:56Z

Dear Pavel Herrmann,

[...]

> >
> > move this out.
> >
> > besides, I think it'd be much systematic to just scream at user to call
> > "sata rescan" and bail out instead of doing it for him.
>
> i dont actually need a sata rescan, i just need to make sure i have
> dynamically allocated names, so i can safely call free() later, otherwise
> this segfaults when called before sata scan

See my previous email

> > > + /* make sure we have valid filenames */
> > > + if (!zeroed) {
> > > + init_sata(0);
> > > + zeroed = 1;
> > > + }
> > > +
> > > + switch (argc) {
> > > + case 0:
> > > + case 1:
> > > + return CMD_RET_USAGE;
> > > + case 2:
> > > + dev = simple_strtoul(argv[1], NULL, 10);
> >
> > Ok, so if I run this command and ask for device 0xb00bf33d ... will this
> > survive? Hint: it won't, rangecheck missing.
>
> hint - scan_sata does the range check in this codepath

saw it, see below. Duplication of code is not good and is error prone.

> > > + return scan_sata(dev);
> > > +
> > > + case 3:
> > > + if (!strncmp(argv[1], "inf", 3)) {
> > > + dev = simple_strtoul(argv[2], NULL, 10);
> >
> > Same here
>
> see above
>
> > > + return scan_sata(dev);
> > > + }
> > > + return CMD_RET_USAGE;
> > > + case 4:
> > > + if (!strncmp(argv[1], "load", 4)) {
> > > + dev = simple_strtoul(argv[2], NULL, 10);
> > > + if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
> >
> > And here you have it ?
> >
> > Uh oh, I see, sata_scan() does it for you ... I'd say, abstract it out
> > into lightweight static inline function.
> >
> > > + printf("File index %d is out of range.\n", dev);
> > > + return -EINVAL;
> > > + }
> > > + free(filenames[dev]);
> > > + filenames[dev] = strdup(argv[3]);
> > > + init_sata(dev);
> > > + return scan_sata(dev);
> > > + }
> > > + return CMD_RET_USAGE;
> > > + }
> > > + return CMD_RET_USAGE;
> > > +}
> > > +
> > > +U_BOOT_CMD(
> > > + sata_loop, 4, 1, do_loop,
> > > + "SATA loopback",
> > > + "[info] devnum - show info about loop devnum\n"
> >
> > Make this "info" part mandatory. Than you can cut the whole argc loop
> > into simple "if argc != 2 ; then fail" . And do simple checking for the
> > first letter of the argument being either i or d .
>
> wont help, still need argc 3 or 4

Makes is simpler, you can bail out if it's not 3 or 4

> > > + "sata_loop load devnum file - load file from host FS into loop
> > > devnum"
> >
> > sata_loop is redundant above.
>
> really? how do you figure?

Run "help sata_loop" and see the result ...

>
> Pavel Herrmann