1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/3] Couple of build fixes after sparse streams
  4. View patch

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

Michal Privoznik posted 3 patches 7 years, 12 months ago
[libvirt] [PATCH 1/3] virfile: Provide stub for virFileInData
Posted by Michal Privoznik 7 years, 12 months ago 
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@redhat.com>
---
 configure.ac       |  5 +++++
 src/util/virfile.c | 15 +++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/configure.ac b/configure.ac
index f20b9ea4d..2e6051354 100644
--- a/configure.ac
+++ b/configure.ac
@@ -352,6 +352,11 @@ AC_CHECK_DECLS([ETH_FLAG_TXVLAN, ETH_FLAG_NTUPLE, ETH_FLAG_RXHASH, ETH_FLAG_LRO,
   [], [], [[#include <linux/ethtool.h>
   ]])
 
+AC_CHECK_DECLS([SEEK_HOLE], [], [],
+               [#include <sys/types.h>
+                #include <unistd.h>])
+
+
 dnl Our only use of libtasn1.h is in the testsuite, and can be skipped
 dnl if the header is not present.  Assume -ltasn1 is present if the
 dnl header could be found.
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 2f4bc42b6..b7645fbfc 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -3798,6 +3798,7 @@ virFileComparePaths(const char *p1, const char *p2)
 }
 
 
+#if HAVE_DECL_SEEK_HOLE
 /**
  * virFileInData:
  * @fd: file to check
@@ -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;
+}
+
+#endif /* !HAVE_DECL_SEEK_HOLE */
+
 
 /**
  * virFileReadValueInt:
-- 
2.13.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] virfile: Provide stub for virFileInData
Posted by Peter Krempa 7 years, 12 months ago 
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@redhat.com>
> ---
>  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?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] virfile: Provide stub for virFileInData
Posted by Michal Privoznik 7 years, 12 months ago 
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@redhat.com>
>> ---
>>  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).

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] virfile: Provide stub for virFileInData
Posted by Peter Krempa 7 years, 12 months ago 
On Fri, May 19, 2017 at 13:31:32 +0200, Michal Privoznik wrote:
> 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@redhat.com>
> >> ---
> >>  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).

Ok, fair enough.

ACK
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] virfile: Provide stub for virFileInData
Posted by Daniel P. Berrange 7 years, 12 months ago 
On Fri, May 19, 2017 at 01:31:32PM +0200, Michal Privoznik wrote:
> 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@redhat.com>
> >> ---
> >>  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).

In particular by reporting an error we give options to the mgmt application.
They may well *not* want to do the upload/download if there is no sparse
support. By reporting an error they can detect that and make a decision
as to whether to retry without sparse flag enabled or not.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.