On Mon, 2014-04-07 at 14:00 -0400, Jeff Layton wrote: > On Sun, 06 Apr 2014 22:28:12 +0100 > Ben Hutchings <[email protected]> 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.
signature.asc
Description: This is a digitally signed message part
