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
+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)
- 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
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)
- 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.
- 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
Best Regards Pavel Herrmann