On Thursday 30 August 2012 23:53:58 Marek Vasut wrote:
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?
because of the silent "*sizeof(sata_dev_desc)". I know this is standardized in C (so is the order of operands), but doing "+" on non-numbers is a little too C++ for me. I know that generated code will be eactly the same in all cases.
+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 ?
the only way init can "fail" is if it gets a wrong device number (which should not happen), or if it cannot open the file, in which case it still sets pdev as -1.
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() mayreturn 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 at all. The last sentence resembles it, but in that case, the behavior is undefined. Can you elaborate please?
RETURN VALUE ... On error, -1 is returned, and errno is set appropriately. ... ERRORS ... EBADF fd is not a valid file descriptor or is not open for writing. ... -1 is definitely not a valid file descriptor. this point is moot, as checking success of lseek (because of pipes/sockets) will filter out invalid fd as well
- 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.
that still wouldn't solve the problem, flename can still be over 20 bytes long
- 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?
I'd like to know that the device is a loopback, and what filename, not just that it failed to init
Pavel Herrmann