From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Thu, 30 Aug 2012 23:53:58 +0200
Subject: [U-Boot] [PATCH] Loop block device for sandbox
In-Reply-To: <2364450.A9Hnn5S86A@bloomfield>
References: <1346255203-1225-1-git-send-email-morpheus.ibis@gmail.com>
	<201208302045.13261.marex@denx.de> <2364450.A9Hnn5S86A@bloomfield>
Message-ID: <201208302353.59126.marex@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Dear Pavel Herrmann,

> On Thursday 30 of August 2012 20:45:13 Marek Vasut wrote:
> > 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 ?
> 
> even less intuitive

Why so?

> > > > > +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.
> 
> init_sata is called first, so pdev is inited (see cmd_sata.c)

Unless it fails. Then what ?

> > > > > +	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.
> 
> shouldn't do that - means i agree there should be a check in case you are
> actively trying to break things, and use pipes/sockets as loop blocks

Good

> > > in the second, the I/O op will harmlessly
> > > fail as well
> > 
> > How so?
> 
> because then the fd is -1, and read/write will do the right thing there
> (nothing, return -1 and set errno to EBADF)

>From write(2)

-->8--

RETURN VALUE
       On  success,  the number of bytes written is returned (zero indicates 
nothing was written).  On error, -1 is returned,
       and errno is set appropriately.

       If count is zero and fd refers to a regular file, then write() may return 
a failure status if one of the errors  below
       is detected.  If no errors are detected, 0 will be returned without 
causing any other effect.  If count is zero and fd
       refers to a file other than a regular file, the results are not 
specified.

--8<--

I don't see the case where fd = -1 handled there@all. The last sentence 
resembles it, but in that case, the behavior is undefined. Can you elaborate 
please?

> > > > > +	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" .
> 
> yes, i was thinking about "...${last 17 bytes of the name}" if the name was
> longer, but this proved significantly simpler for demonstrating the general
> idea.

I think the FS code might contain some function to fixup the path and get 
filename from path.

> > > > > +	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
> 
> not the case. sata_dev_desc is inited in cmd_sata.c, and therefore by not
> doing anything we get an empty device

I see ... shall we also move all these memcpy() calls in to if (fd != -1) then?

> Best Regards
> Pavel Herrmann

Best regards,
Marek Vasut