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

[Marek Vasut <marex@denx.de>]

Dear Pavel Herrmann,

> On Thursday 06 of September 2012 03:08:42 Marek Vasut wrote:
> > Dear Pavel Herrmann,
> > 
> > > On Wednesday 05 of September 2012 14:48:40 Marek Vasut wrote:
> > > > Dear Pavel Herrmann,
> > > > 
> > > > [...]
> > > > 
> > > > > > 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
> > > > 
> > > > Sorry, I can't parse this ... but ...
> > > > 
> > > > > , so i can safely call free() later, otherwise
> > > > > this segfaults when called before sata scan
> > > > 
> > > > The free() function frees the memory space pointed to by ptr, which
> > > > must have been returned by a previous call to malloc(), calloc() or
> > > > realloc().
> > > > Otherwise, or if free(ptr) has already been called before, undefined
> > > > behavior  occurs. If ptr is NULL, no operation is performed.
> > > > 
> > > > So if you call free() on null pointer, nothing happens. Where's the
> > > > real problem?
> > > 
> > > if you called "sata init" before setting all the loops you would get to
> > > open(NULL) and a strlen(NULL). i think its easier to supply valid empty
> > > string then check for NULL at every access.
> > 
> > I'd say check for null on every access ... you'd lower memory
> > requirements and the increase in code requirement should really be very
> > minor. Also, it'd remove the "zeroed" static variable altogether,
> > correct?
> 
> OK, fine by me
> 
> > > > > > 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
> > > 
> > > still, i should have a "sata_loop info" work on all files, so theres a
> > > valid command with argc 2
> > 
> > Understood.
> > 
> > > > > > > +	"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 ...
> > > 
> > > =>help sata_loop
> > > sata_loop - SATA loopback
> > > 
> > > Usage:
> > > sata_loop [info] devnum - show info about loop devnum
> > > sata_loop load devnum file - load file from host FS into loop devnum
> > > 
> > > i dont see your problem
> > 
> > Ewww ... nevermind ... I just detected yet another stupid issue in the
> > codebase. I'd say this "sata_loop" should be either omitted and filled by
> > the macro or something. But that's for other time to solve ... so I'm
> > fine with this now.
> 
> it actually is, but only to the first line (as both lines are in one
> string). In my opinion it should either add this for each line (with some
> magic string modification) or none at all, to keep it consistent

Keep it as is now, but it's something that should eventually be resolved.

> Pavel Herrmann