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

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.