8:46 a.m.
Some systems lack the symbols we need or have them in different header files.
Michal Privoznik (3):
virfile: Provide stub for virFileInData
virfiletest: Test virFileInData iff SEEK_HOLE is defined
virfiletest: include linux/falloc.h
configure.ac | 5 +++++
src/util/virfile.c | 15 +++++++++++++++
tests/virfiletest.c | 40 +++++++++++++++++++++++++++-------------
3 files changed, 47 insertions(+), 13 deletions(-)
--
2.13.0
8:46 a.m.
New subject: [libvirt] [PATCH 1/3] virfile: Provide stub for virFileInData
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>
---
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
5:28 a.m.
New subject: [libvirt] [PATCH 1/3] virfile: Provide stub for virFileInData
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>
---
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?
6:31 a.m.
New subject: [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>
> ---
> 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
6:57 a.m.
New subject: [libvirt] [PATCH 1/3] virfile: Provide stub for virFileInData
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(a)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
7 a.m.
New subject: [libvirt] [PATCH 1/3] virfile: Provide stub for virFileInData
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(a)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 :|
8:46 a.m.
New subject: [libvirt] [PATCH 2/3] virfiletest: Test virFileInData iff SEEK_HOLE is defined
Yet another place where we need to wrap code in
HAVE_DECL_SEEK_HOLE block.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tests/virfiletest.c | 36 +++++++++++++++++++++++-------------
1 file changed, 23 insertions(+), 13 deletions(-)
diff --git a/tests/virfiletest.c b/tests/virfiletest.c
index a93bee01a..d6a526c13 100644
--- a/tests/virfiletest.c
+++ b/tests/virfiletest.c
@@ -123,7 +123,11 @@ static int
makeSparseFile(const off_t offsets[],
const bool startData);
-#ifdef __linux__
+static bool
+holesSupported(void);
+
+#if HAVE_DECL_SEEK_HOLE
+
/* Create a sparse file. @offsets in KiB. */
static int
makeSparseFile(const off_t offsets[],
@@ -188,19 +192,8 @@ makeSparseFile(const off_t offsets[],
return -1;
}
-#else /* !__linux__ */
-static int
-makeSparseFile(const off_t offsets[] ATTRIBUTE_UNUSED,
- const bool startData ATTRIBUTE_UNUSED)
-{
- return -1;
-}
-
-#endif /* !__linux__ */
-
-
-#define EXTENT 4
+# define EXTENT 4
static bool
holesSupported(void)
{
@@ -245,6 +238,23 @@ holesSupported(void)
return ret;
}
+#else /* !HAVE_DECL_SEEK_HOLE */
+
+static int
+makeSparseFile(const off_t offsets[] ATTRIBUTE_UNUSED,
+ const bool startData ATTRIBUTE_UNUSED)
+{
+ return -1;
+}
+
+
+static bool
+holesSupported(void)
+{
+ return false;
+}
+
+#endif /* !HAVE_DECL_SEEK_HOLE */
struct testFileInData {
bool startData; /* whether the list of offsets starts with data section */
--
2.13.0
5:34 a.m.
New subject: [libvirt] [PATCH 2/3] virfiletest: Test virFileInData iff SEEK_HOLE is defined
On Thu, May 18, 2017 at 15:46:45 +0200, Michal Privoznik wrote:
Yet another place where we need to wrap code in
HAVE_DECL_SEEK_HOLE block.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tests/virfiletest.c | 36 +++++++++++++++++++++++-------------
1 file changed, 23 insertions(+), 13 deletions(-)
diff --git a/tests/virfiletest.c b/tests/virfiletest.c
index a93bee01a..d6a526c13 100644
--- a/tests/virfiletest.c
+++ b/tests/virfiletest.c
@@ -123,7 +123,11 @@ static int
makeSparseFile(const off_t offsets[],
const bool startData);
-#ifdef __linux__
+static bool
+holesSupported(void);
This is not necessary.
+
+#if HAVE_DECL_SEEK_HOLE
+
/* Create a sparse file. @offsets in KiB. */
static int
makeSparseFile(const off_t offsets[],
ACK
5:36 a.m.
New subject: [libvirt] [PATCH 2/3] virfiletest: Test virFileInData iff SEEK_HOLE is defined
On Thu, May 18, 2017 at 15:46:45 +0200, Michal Privoznik wrote:
Yet another place where we need to wrap code in
HAVE_DECL_SEEK_HOLE block.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tests/virfiletest.c | 36 +++++++++++++++++++++++-------------
1 file changed, 23 insertions(+), 13 deletions(-)
diff --git a/tests/virfiletest.c b/tests/virfiletest.c
index a93bee01a..d6a526c13 100644
--- a/tests/virfiletest.c
+++ b/tests/virfiletest.c
@@ -123,7 +123,11 @@ static int
makeSparseFile(const off_t offsets[],
const bool startData);
-#ifdef __linux__
+static bool
+holesSupported(void);
+
+#if HAVE_DECL_SEEK_HOLE
Also this stubbed out code is now dependant on HAVE_DECL_SEEK_HOLE, but
the next patch includes file necessary for FALLOC_FL_PUNCH_HOLE only if
__linux__ is defined. So you probably want HAVE_DECL_SEEK_HOLE &&
__linux__
6:31 a.m.
New subject: [libvirt] [PATCH 2/3] virfiletest: Test virFileInData iff SEEK_HOLE is defined
On 05/19/2017 12:36 PM, Peter Krempa wrote:
On Thu, May 18, 2017 at 15:46:45 +0200, Michal Privoznik wrote:
> Yet another place where we need to wrap code in
> HAVE_DECL_SEEK_HOLE block.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> tests/virfiletest.c | 36 +++++++++++++++++++++++-------------
> 1 file changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/tests/virfiletest.c b/tests/virfiletest.c
> index a93bee01a..d6a526c13 100644
> --- a/tests/virfiletest.c
> +++ b/tests/virfiletest.c
> @@ -123,7 +123,11 @@ static int
> makeSparseFile(const off_t offsets[],
> const bool startData);
>
> -#ifdef __linux__
> +static bool
> +holesSupported(void);
> +
> +#if HAVE_DECL_SEEK_HOLE
Also this stubbed out code is now dependant on HAVE_DECL_SEEK_HOLE, but
the next patch includes file necessary for FALLOC_FL_PUNCH_HOLE only if
__linux__ is defined. So you probably want HAVE_DECL_SEEK_HOLE &&
__linux__
Okay.
Michal
8:46 a.m.
New subject: [libvirt] [PATCH 3/3] virfiletest: include linux/falloc.h
On systems with older glibc including fcntl.h for getting
FALLOC_FL_PUNCH_HOLE defined is not enough. We must also include
linux/falloc.h.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tests/virfiletest.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/tests/virfiletest.c b/tests/virfiletest.c
index d6a526c13..d5bded00e 100644
--- a/tests/virfiletest.c
+++ b/tests/virfiletest.c
@@ -27,6 +27,10 @@
#include "virfile.h"
#include "virstring.h"
+#ifdef __linux__
+# include <linux/falloc.h>
+#endif
+
#if defined HAVE_MNTENT_H && defined HAVE_GETMNTENT_R
static int testFileCheckMounts(const char *prefix,
--
2.13.0
5:37 a.m.
New subject: [libvirt] [PATCH 3/3] virfiletest: include linux/falloc.h
On Thu, May 18, 2017 at 15:46:46 +0200, Michal Privoznik wrote:
On systems with older glibc including fcntl.h for getting
FALLOC_FL_PUNCH_HOLE defined is not enough. We must also include
linux/falloc.h.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tests/virfiletest.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/tests/virfiletest.c b/tests/virfiletest.c
index d6a526c13..d5bded00e 100644
--- a/tests/virfiletest.c
+++ b/tests/virfiletest.c
@@ -27,6 +27,10 @@
#include "virfile.h"
#include "virstring.h"
+#ifdef __linux__
+# include <linux/falloc.h>
+#endif
ACK if you tweak the previous commit as described.
2744
days inactive
2745
days old
11 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Michal Privoznik -
Peter Krempa