[libvirt] [PATCH 0/3] Fix Mac OS X build issues (and probably also BSD)
While compiling libvirt on a Mac OS X machine, the build failed tue to several reasons. This patchset fixes these issues and along with another patch that removes shadowed declarations of "devname" from stdlib.h on BSD based systems causes libvirt to compile cleanly. These patches were tested on v0.9.5-rc2 as I don't have the complete toolchain to do ./autogen.sh on a gitcheckout. Problems found: - fdatasync() is not supported on OS X. - Conditionaly compiled structure was left out but function using it was not. - OS X does not provide a 'mkfs' command, so the MKFS macro was not set. - /usr/include/stdlib.h exports 'devname' wildly used through libvirt Peter Krempa (3): build: storage: fdatasync() is not supported in Mac OS X. build: storage: Conditionaly compiled structure caused build fail on OSX build: storage: Macro 'MKFS' is undefined on some platforms. src/storage/storage_backend.c | 2 +- src/storage/storage_backend_fs.c | 15 +++++++++++++++ src/storage/storage_backend_fs.h | 3 ++- src/storage/storage_driver.c | 2 +- 4 files changed, 19 insertions(+), 3 deletions(-) -- 1.7.3.4
This patch changes fdatasync() to fsync() to compile cleanly in OS X. --- src/storage/storage_backend.c | 2 +- src/storage/storage_driver.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index d125504..ee39836 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -208,7 +208,7 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, } while ((amtleft -= interval) > 0); } - if (fdatasync(fd) < 0) { + if (fsync(fd) < 0) { ret = -errno; virReportSystemError(errno, _("cannot sync data to file '%s'"), vol->target.path); diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index c05b74e..aea4c71 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1783,7 +1783,7 @@ storageWipeExtent(virStorageVolDefPtr vol, remaining -= written; } - if (fdatasync(fd) < 0) { + if (fsync(fd) < 0) { ret = -errno; virReportSystemError(errno, _("cannot sync data to volume with path '%s'"), -- 1.7.3.4
On Fri, Sep 16, 2011 at 02:14:14PM +0200, Peter Krempa wrote:
This patch changes fdatasync() to fsync() to compile cleanly in OS X. --- src/storage/storage_backend.c | 2 +- src/storage/storage_driver.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index d125504..ee39836 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -208,7 +208,7 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, } while ((amtleft -= interval) > 0); }
- if (fdatasync(fd) < 0) { + if (fsync(fd) < 0) { ret = -errno; virReportSystemError(errno, _("cannot sync data to file '%s'"), vol->target.path); diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index c05b74e..aea4c71 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1783,7 +1783,7 @@ storageWipeExtent(virStorageVolDefPtr vol, remaining -= written; }
- if (fdatasync(fd) < 0) { + if (fsync(fd) < 0) { ret = -errno; virReportSystemError(errno, _("cannot sync data to volume with path '%s'"),
Unfortunately they don't have the same semantic, and I assume that if we used fdatasync it was to avoid sync'ing all the metadata which fsync does. The proper way would be to make a configure time check for fdatasync and if not present fallback to fsync. A function in util/file.[ch] called virFileDataSync(int fd) which would do fdatasync(fd) if available and fsync(fd) as a fallback sounds to me the best way to achieve this. I don't think we should really push this as-is, but if someone else disagrees for the sake of 0.9.5 portability, fine. Or we can delay 0.9.5 until fixed for good (I planned to make rc3 within hours and the final release Monday), Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 09/16/2011 06:14 AM, Peter Krempa wrote:
This patch changes fdatasync() to fsync() to compile cleanly in OS X.
I don't like this; it's too heavy-handed on working platforms. Rather, we should provide a gnulib wrapper function for fdatasync() that falls back on fsync() when compiled on MacOS. I'll get that done today (although for the purposes of rc3, if DV would rather push this patch now, and revert it later when the gnulib changes are complete, so that rc3 can go out before I get things done, that would work too). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, Sep 16, 2011 at 07:04:31AM -0600, Eric Blake wrote:
On 09/16/2011 06:14 AM, Peter Krempa wrote:
This patch changes fdatasync() to fsync() to compile cleanly in OS X.
I don't like this; it's too heavy-handed on working platforms. Rather, we should provide a gnulib wrapper function for fdatasync() that falls back on fsync() when compiled on MacOS. I'll get that done today (although for the purposes of rc3, if DV would rather push this patch now, and revert it later when the gnulib changes are complete, so that rc3 can go out before I get things done, that would work too).
What about a wrapper ? virFileDataSync as I sugegsted, it's still simpler than a new gnulib function, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Mingw lacks fsync, but gnulib provides that. Meanwhile, gnulib does not (yet) provide fdatasync, so this is a quick hack to fake that function on MacOS X; we can revert this configure change once gnulib gives us a real module. * bootstrap.conf (gnulib_modules): Add fsync. * configure.ac (AC_CHECK_FUNCS_ONCE): Check for fdatasync, and fake it with fsync when not present. --- I've tested that this is a no-op on Linux, but would appreciate someone with a MacOS setup to quickly test that it fixes the build there. bootstrap.conf | 1 + configure.ac | 5 ++++- 2 files changed, 5 insertions(+), 1 deletions(-) diff --git a/bootstrap.conf b/bootstrap.conf index 94af051..e609ba8 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -42,6 +42,7 @@ fcntl fcntl-h ffs fnmatch +fsync func getaddrinfo getcwd-lgpl diff --git a/configure.ac b/configure.ac index 5e3539f..611a141 100644 --- a/configure.ac +++ b/configure.ac @@ -126,9 +126,12 @@ AC_CHECK_SIZEOF([long]) dnl Availability of various common functions (non-fatal if missing), dnl and various less common threadsafe functions -AC_CHECK_FUNCS_ONCE([cfmakeraw geteuid getgid getgrnam_r getmntent_r \ +AC_CHECK_FUNCS_ONCE([cfmakeraw fdatasync geteuid getgid getgrnam_r getmntent_r \ getpwuid_r getuid initgroups kill mmap posix_fallocate posix_memalign \ regexec sched_getaffinity]) +if test $ac_cv_func_fdatasync = no; then + AC_DEFINE([fdatasync], [fsync], [Define to fsync if you lack fdatasync]) +fi dnl Availability of pthread functions (if missing, win32 threading is dnl assumed). Because of $LIB_PTHREAD, we cannot use AC_CHECK_FUNCS_ONCE. -- 1.7.4.4
On 09/16/2011 09:21 AM, Eric Blake wrote:
Mingw lacks fsync, but gnulib provides that. Meanwhile, gnulib does not (yet) provide fdatasync, so this is a quick hack to fake that function on MacOS X; we can revert this configure change once gnulib gives us a real module.
* bootstrap.conf (gnulib_modules): Add fsync. * configure.ac (AC_CHECK_FUNCS_ONCE): Check for fdatasync, and fake it with fsync when not present. ---
I've tested that this is a no-op on Linux, but would appreciate someone with a MacOS setup to quickly test that it fixes the build there.
I've finally got to the point where I could do some more testing, and this does indeed fix the build error for MacOS, so I'm pushing under the build-breaker rule so that it will be in rc3. I've also worked on several gnulib changes to add a fdatasync module: https://lists.gnu.org/archive/html/bug-gnulib/2011-09/msg00205.html In particular, that includes a patch to use the available (but undeclared) fdatasync added in MacOS X 10.7 (the libvirt patch above is intentionally dumber, and blindly uses fsync even on MacOS 10.7): https://lists.gnu.org/archive/html/bug-gnulib/2011-09/msg00217.html However, I won't do a .gnulib submodule update to pick up any of those changes until after 0.9.5 is released, especially since the gnulib side might still be under heavy churn this weekend as it gets tested on various platforms. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Struct virStoragePoolProbeResult was compiled in conditionaly, but virStorageBackendFileSystemProbe used it unconditionaly. This patch exempts the struct from conditional include. --- src/storage/storage_backend_fs.h | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/storage/storage_backend_fs.h b/src/storage/storage_backend_fs.h index 54c6739..4f69056 100644 --- a/src/storage/storage_backend_fs.h +++ b/src/storage/storage_backend_fs.h @@ -29,12 +29,13 @@ # if WITH_STORAGE_FS extern virStorageBackend virStorageBackendFileSystem; extern virStorageBackend virStorageBackendNetFileSystem; +# endif + typedef enum { FILESYSTEM_PROBE_FOUND, FILESYSTEM_PROBE_NOT_FOUND, FILESYSTEM_PROBE_ERROR, } virStoragePoolProbeResult; -# endif extern virStorageBackend virStorageBackendDirectory; #endif /* __VIR_STORAGE_BACKEND_FS_H__ */ -- 1.7.3.4
On Fri, Sep 16, 2011 at 02:14:15PM +0200, Peter Krempa wrote:
Struct virStoragePoolProbeResult was compiled in conditionaly, but virStorageBackendFileSystemProbe used it unconditionaly. This patch exempts the struct from conditional include. --- src/storage/storage_backend_fs.h | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/storage/storage_backend_fs.h b/src/storage/storage_backend_fs.h index 54c6739..4f69056 100644 --- a/src/storage/storage_backend_fs.h +++ b/src/storage/storage_backend_fs.h @@ -29,12 +29,13 @@ # if WITH_STORAGE_FS extern virStorageBackend virStorageBackendFileSystem; extern virStorageBackend virStorageBackendNetFileSystem; +# endif + typedef enum { FILESYSTEM_PROBE_FOUND, FILESYSTEM_PROBE_NOT_FOUND, FILESYSTEM_PROBE_ERROR, } virStoragePoolProbeResult; -# endif extern virStorageBackend virStorageBackendDirectory;
#endif /* __VIR_STORAGE_BACKEND_FS_H__ */
Nice bug :-) ACK, I'm gonna push it, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Mac OS X 10.6. Snow Leopard and probably other do not provide a mkfs command to create filesystems. Macro MKFS then remained undefined and did not provide any substitute, so that build failed on a missing argument. --- src/storage/storage_backend_fs.c | 15 +++++++++++++++ 1 files changed, 15 insertions(+), 0 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 02c4c17..da98f87 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -625,6 +625,8 @@ virStorageBackendFileSystemProbe(const char *device ATTRIBUTE_UNUSED, #endif /* #if HAVE_LIBBLKID */ +/* some platforms don't support mkfs */ +#ifdef MKFS static int virStorageBackendExecuteMKFS(const char *device, const char *format) @@ -647,6 +649,19 @@ virStorageBackendExecuteMKFS(const char *device, } return ret; } +#else /* #ifdef MKFS */ +static int +virStorageBackendExecuteMKFS(const char *device ATTRIBUTE_UNUSED, + const char *format ATTRIBUTE_UNUSED) +{ + virStorageReportError(VIR_ERR_INTERNAL_ERROR, + _("mkfs is not supported on this platform: " + "Failed to make filesystem of " + "type '%s' on device '%s'"), + format, device); + return -1; +} +#endif /* #ifdef MKFS */ static int virStorageBackendMakeFileSystem(virStoragePoolObjPtr pool, -- 1.7.3.4
On Fri, Sep 16, 2011 at 02:14:16PM +0200, Peter Krempa wrote:
Mac OS X 10.6. Snow Leopard and probably other do not provide a mkfs command to create filesystems. Macro MKFS then remained undefined and did not provide any substitute, so that build failed on a missing argument. --- src/storage/storage_backend_fs.c | 15 +++++++++++++++ 1 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 02c4c17..da98f87 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -625,6 +625,8 @@ virStorageBackendFileSystemProbe(const char *device ATTRIBUTE_UNUSED,
#endif /* #if HAVE_LIBBLKID */
+/* some platforms don't support mkfs */ +#ifdef MKFS static int virStorageBackendExecuteMKFS(const char *device, const char *format) @@ -647,6 +649,19 @@ virStorageBackendExecuteMKFS(const char *device, } return ret; } +#else /* #ifdef MKFS */ +static int +virStorageBackendExecuteMKFS(const char *device ATTRIBUTE_UNUSED, + const char *format ATTRIBUTE_UNUSED) +{ + virStorageReportError(VIR_ERR_INTERNAL_ERROR, + _("mkfs is not supported on this platform: " + "Failed to make filesystem of " + "type '%s' on device '%s'"), + format, device); + return -1; +} +#endif /* #ifdef MKFS */
static int virStorageBackendMakeFileSystem(virStoragePoolObjPtr pool,
Okay, I think this is teh right way, ACK, I'm gonna push your patch set except for 1/3, thanks a lot ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (3)
-
Daniel Veillard -
Eric Blake -
Peter Krempa