[libvirt] [PATCH] avoid a probable EINVAL from lseek

[Daniel P. Berrange]

On Tue, Feb 02, 2010 at 11:01:55AM +0100, Jim Meyering wrote:
> Daniel P. Berrange wrote:
> > On Tue, Feb 02, 2010 at 10:37:32AM +0100, Jim Meyering wrote:
> >> Daniel P. Berrange wrote:
> >>
> >> > On Mon, Feb 01, 2010 at 10:18:27PM +0100, Jim Meyering wrote:
> >> >>
> >> >> In src/qemu/qemu_driver.c, coverity reports this:
> >> >>
> >> >>   Event negative_return_fn: Called negative-returning function "lseek(logfile, 0L, 2)"
> >> >>   Event var_assign: NEGATIVE return value of "lseek" assigned to signed variable "pos"
> >> >>   At conditional (1): "(pos = lseek(logfile, 0L, 2)) < 0" taking true path
> >> >>   2877 	    if ((pos = lseek(logfile, 0, SEEK_END)) < 0)
> >> >>   2878 	        VIR_WARN(_("Unable to seek to end of logfile: %s"),
> >> >>   2879 	                 virStrerror(errno, ebuf, sizeof ebuf));
> >> >
> >> > I think it'd be less surprising to just set 'pos = 0' inside the if
> >> > branch here, so later code doesn't have to worry about unexpected
> >> > negative values.
> >>
> >> Oh.  I pushed after DV's ACK.
> >>
> >> That would let the later code continue, but using (lseek'ing to) an
> >> invalid position.  Sounds like it could result in a cascade of
> >> additional errors, or worse, silent malfunction.
> >
> > The code which later uses this 'pos'  variable, does so in order to skip
> > over the start of the logfile which it does not want. So by setting pos=0
> > we make it start at the beginning of the file instead.
> 
> Wouldn't that result in reprocessing (misleadingly) past diagnostics?

No, all that its doing is skipping over the ARGV that were written to
the start of the file, to make future error reporting slightly cleaner.

> >> But this is largely hypothetical, since failing to lseek-to-EOF
> >> on a valid file descriptor is not likely to happen.
> >
> > My reasoning is that putting the 'pos < 0' check in another separate
> > method far away from where the error occurs is rather obscure, and
> > the kind of code that will likely be deleted by mistake in later
> > refactoring.
> 
> I agree that this action-at-a-distance is bad.
> I can add a comment pointing to this discussion.
> 
> Or,... considering that this type of failure is so unlikely,
> would you prefer to make the initial lseek evoke failure
> rather than just a warning?

I'd really rather we just reset pos to 0

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|