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.
> 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.
>
> >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> >> index 22593bf..676a27b 100644
> >> --- a/src/qemu/qemu_driver.c
> >> +++ b/src/qemu/qemu_driver.c
> >> @@ -558,8 +558,8 @@ qemudLogReadFD(virConnectPtr conn, const char* logDir, const char* name, off_t p
> >> close(fd);
> >> return -1;
> >> }
> >> - if (lseek(fd, pos, SEEK_SET) < 0) {
> >> - virReportSystemError(conn, errno,
> >> + if (pos < 0 || lseek(fd, pos, SEEK_SET) < 0) {
> >> + virReportSystemError(conn, pos < 0 ? 0 : errno,
> >> _("Unable to seek to %lld in %s"),
> >> (long long) pos, logfile);
> >> close(fd);
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list