[libvirt] [PATCH 0/3] Couple of build fixes after sparse streams
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
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
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?
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
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
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 :|
Yet another place where we need to wrap code in HAVE_DECL_SEEK_HOLE block. Signed-off-by: Michal Privoznik <mprivozn@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
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@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
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@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__
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@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
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@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
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@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.
participants (3)
-
Daniel P. Berrange -
Michal Privoznik -
Peter Krempa