Re: [libvirt] [PATCH 1/3] virfile: Provide stub for virFileInData

On 05/19/2017 12:28 PM, Peter Krempa wrote: > On Thu, May 18, 2017 at 15:46:44 +0200, Michal Privoznik wrote: >> Some older systems (such as RHEL6) lack SEEK_HOLE and SEEK_DATA >> which virFileInData relies on. Provide a stub for these systems. >> >> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >> --- >> configure.ac | 5 +++++ >> src/util/virfile.c | 15 +++++++++++++++ >> 2 files changed, 20 insertions(+) > > [...] > >> @@ -3904,6 +3905,20 @@ virFileInData(int fd, >> return ret; >> } >> >> +#else /* !HAVE_DECL_SEEK_HOLE */ >> + >> +int >> +virFileInData(int fd ATTRIBUTE_UNUSED, >> + int *inData ATTRIBUTE_UNUSED, >> + long long *length ATTRIBUTE_UNUSED) >> +{ >> + virReportSystemError(ENOSYS, "%s", >> + _("sparse files not supported")); >> + return -1; > > Wouldn't it be better as a fallback always return that data is present > rather than failing? > I don't think so. What I can do actually is to not only report error, but also set errno = ENOSYS, so that caller can distinguish this case from other cases where virFileInData fails. I think we should let it up to the caller to decide then whether they want to ignore the error (and pretend there are no holes), or fail too. More generally, helpers in src/util/ should really be one purpose functions without trying to be clever and leave the logic for callers (drivers).