[U-Boot] [PATCH] Loop block device for sandbox

[Marek Vasut]

Dear Pavel Herrmann,

> On Thursday 30 of August 2012 00:18:18 Marek Vasut wrote:
> ...snip...
> 
> > > +extern block_dev_desc_t sata_dev_desc[];
> > > +
> > > +int init_sata(int dev)
> > > +{
> > > +	block_dev_desc_t *pdev = &(sata_dev_desc[dev]);
> > 
> > Superfluous braces ... Actually, I think sata_dev_desc as it would work
> > very well too.
> 
> Straight copy from dwc_ahsata.c, makes it more readable thought, as the
> order of operation is not very intuitive IMHO.

sata_dev_desc + dev ?

> > > +lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt, void
> > > *buffer)
> > > +{
> > > +	block_dev_desc_t *pdev = &(sata_dev_desc[dev]);
> > > +	int fd = (long) pdev->priv;
> > 
> > If pdev is NULL, this will crash
> 
> well, it isn't, at least not from the command - thats why you define the
> number of ports in advance, you get "dev" already range-checked

Range check is fine, but will pdev be inited? It's a pointer from some array.

> > > +	lbaint_t retval;
> > > +
> > > +	os_lseek(fd, start*ATA_SECT_SIZE, OS_SEEK_SET);
> > > +	retval = os_read(fd, buffer, ATA_SECT_SIZE * blkcnt);
> > > +
> > > +	return retval/ATA_SECT_SIZE;
> > > +}
> > > +
> > > +lbaint_t sata_write(int dev, lbaint_t start, lbaint_t blkcnt, void
> > > *buffer) +{
> > > +	block_dev_desc_t *pdev = &(sata_dev_desc[dev]);
> > > +	int fd = (long) pdev->priv;
> > > +	lbaint_t retval;
> > > +
> > > +	os_lseek(fd, start*ATA_SECT_SIZE, OS_SEEK_SET);
> > 
> > Besides, lseek can fail, can it not?
> 
> If you open a pipe (or nothing), yes
> in the first case, you shouldn't

Shouldn't ... what? Sorry, I cannot parse this.

> in the second, the I/O op will harmlessly
> fail as well

How so?

> > > +	if (namelen > 20)
> > > +		namelen = 20;
> > 
> > Why do you trim down the string, won't simple strdup() work?
> 
> nah, the destination is char[21], as it is the exact length of
> corresponding field in ATA identify response (one more for a 0 at the end)

I see, is it a full path ? If so, it might be a better idea to use the filename 
itself instead of the whole path. So you'd prevent names like 
"~/../foo/../.././bar.img" .

> > > +	memcpy(pdev->product, filenames[dev], namelen);
> > > +	pdev->product[20] = 0;
> > > +
> > > +	if (fd != -1) {
> > 
> > And if "fd" is -1 ?
> 
> then all defaults to an invalid device, because you failed to open the
> file, for whatever the reason.

At least the printf below will choke, since pdev->lba is uninited

> "agreed" to the other comments
> 
> Best Regards
> Pavel Herrmann
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

Best regards,
Marek Vasut