Dear Pavel Herrmann,
> 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.
It's simple increment of a pointer, normal thing.
> > > > > > > +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.
If pdev is -1, then this explodes for certain, on unaligned access and on wrong
ptr deref.
> > > > > 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 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.
I see, good catch.
> 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
Then pick the first 20 ... but this is really discutable
> > > > > > > + 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
But are such data used somewhere further down the road?
> Pavel Herrmann
Best regards,
Marek Vasut
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
