On Mon, 2014-04-07 at 14:00 -0400, Jeff Layton wrote:
> On Sun, 06 Apr 2014 22:28:12 +0100
> Ben Hutchings <ben@xxxxxxxxxxxxxxx> wrote:
>
> > Here's the backported version I've queued up for 3.2. It's untested;
> > please let me know if you see any problem with it.
[...]
> > + /*
> > + * If we have no data to send, then that probably means that
> > + * the copy above failed altogether. That's most likely because
> > + * the address in the iovec was bogus. Set the rc to -EFAULT,
> > + * free anything we allocated and bail out.
> > + */
> > + if (!cur_len) {
> > + kunmap(pages[0]);
> > + rc = -EFAULT;
> > + break;
> > + }
> > +
>
>
> I don't think this looks quite right in 3.2...
>
> If you hit the -EFAULT case above, then you'll break out of the big
> (outer) do...while loop. The code that handles whether to do a short
> write or an error code is in that loop, and that break will bypass it.
>
> If you didn't actually do any I/O at that point, then cifs_iovec_write
> is going to return 0 instead of -EFAULT. You'll probably need to
> rejigger the error handling to make that work properly.
Yes, I fixed that in v2.
> Looks like there's also an existing bug in there too if
> cifs_reopen_file fails...
Right, Raphael also noticed that and I'll post a patch for the next
update.
Ben.
> > + /*
> > + * i + 1 now represents the number of pages we actually used in
> > + * the copy phase above.
> > + */
> > + npages = i + 1;
> > +
> > do {
> > if (open_file->invalidHandle) {
> > rc = cifs_reopen_file(open_file, false);
> >
> >
--
Ben Hutchings
Sturgeon's Law: Ninety percent of everything is crap.
Attachment:
signature.asc
Description: This is a digitally signed message part