On 06/05/2017 04:23 AM, Michal Privoznik wrote: > Callers might be interested in the original value of errno. Let's > not overwrite it with lseek() done in cleanup path. > > Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> > --- > src/util/virfile.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > One could argue that if the original lseek fails, rather than the goto cleanup, the return -1 could be done. That way, the "if" condition in cleanup: isn't necessary... Additionally, if the second if the second lseek fails to reposition back to where we started and we don't tell the user that, then we're off in never never land possibly, right? Of course IIRC the argument about this was that lseek is highly unlikely to fail in that second scenario. Up to this point we don't say/guarantee anything about the errno return value generally, but I see the subsequent patch adds some verbiage - whether it can be accurately held over any libvirt call perhaps remains to be seen. Still the premise of this is, don't mess with an errno that was set to something with a second one that could change the original intention. Before the ACK/R-B applies, is there any odd special casing that needs to be done for that ENXIO case? That is setting errno = 0 if we decide to not goto cleanup as I don't think in that case we're considering an errno value to be problematic, instead it'd be expected. As long as the errno != ENXIO case is handled... Since we learned nothing and are moving on happily. Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx> John > diff --git a/src/util/virfile.c b/src/util/virfile.c > index d444b32f8..2be64f1db 100644 > --- a/src/util/virfile.c > +++ b/src/util/virfile.cf > @@ -3900,8 +3900,11 @@ virFileInData(int fd, > ret = 0; > cleanup: > /* At any rate, reposition back to where we started. */ > - if (cur != (off_t) -1) > + if (cur != (off_t) -1) { > + int save_errno = errno; > ignore_value(lseek(fd, cur, SEEK_SET)); > + errno = save_errno; > + } > return ret; > } > > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list