From: Oliver Kurth Date: Fri, 15 Sep 2017 18:22:47 +0000 (-0700) Subject: Correct the freeze and thaw ordering for mount points X-Git-Tag: stable-10.2.0~678 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=b9382f4cbb636d9663e878a416b5b9727ab42d6f;p=thirdparty%2Fopen-vm-tools.git Correct the freeze and thaw ordering for mount points There were two issues with the way we were doing quiescing on Linux: 1. Thaw was following the same order as freeze, actually it should follow the reverse order of freeze. Fixed the thaw order. 2. Freeze was following the order provided by getmntent API which is the order in which system created the mount points. This could be problematic when a mount point depends on other mount point, e.g. loopback mount point. In order to honor the dependency among mount points, we need to reverse the order of mount points listed by getmntent API. While reviewing this change it was found that the interface used for passing the mount points around was not very clean. It was a ':' separated string of mount points. There were multiple problems with it. We were converting a list of strings into one string and then tokenizing it later. As part of this change, we fix that interface too by replacing the string with single-linked list, GSList. Using GSList brings glib dependency to lib/syncDriver. --- diff --git a/open-vm-tools/lib/syncDriver/Makefile.am b/open-vm-tools/lib/syncDriver/Makefile.am index c7f89427f..699c16ade 100644 --- a/open-vm-tools/lib/syncDriver/Makefile.am +++ b/open-vm-tools/lib/syncDriver/Makefile.am @@ -17,6 +17,9 @@ noinst_LTLIBRARIES = libSyncDriver.la +libSyncDriver_la_CPPFLAGS = +libSyncDriver_la_CPPFLAGS += @GLIB2_CPPFLAGS@ + libSyncDriver_la_SOURCES = libSyncDriver_la_SOURCES += syncDriverPosix.c diff --git a/open-vm-tools/lib/syncDriver/nullDriver.c b/open-vm-tools/lib/syncDriver/nullDriver.c index d4b24bdd0..5e19e2080 100644 --- a/open-vm-tools/lib/syncDriver/nullDriver.c +++ b/open-vm-tools/lib/syncDriver/nullDriver.c @@ -63,7 +63,7 @@ NullDriverClose(SyncDriverHandle handle) */ SyncDriverErr -NullDriver_Freeze(const char *paths, +NullDriver_Freeze(const GSList *paths, SyncDriverHandle *handle) { /* diff --git a/open-vm-tools/lib/syncDriver/syncDriverInt.h b/open-vm-tools/lib/syncDriver/syncDriverInt.h index 9fdb03a1f..79c56f800 100644 --- a/open-vm-tools/lib/syncDriver/syncDriverInt.h +++ b/open-vm-tools/lib/syncDriver/syncDriverInt.h @@ -25,19 +25,22 @@ * Internal definitions for the sync driver library. */ +#include #include "syncDriver.h" #define LGPFX "SyncDriver: " #if !defined(Win32) +#define SYNCDRIVER_PATH_SEPARATOR ':' + typedef enum { SD_SUCCESS, SD_ERROR, SD_UNAVAILABLE, } SyncDriverErr; -typedef SyncDriverErr (*SyncFreezeFn)(const char *paths, +typedef SyncDriverErr (*SyncFreezeFn)(const GSList *paths, SyncDriverHandle *handle); typedef struct SyncHandle { @@ -47,15 +50,15 @@ typedef struct SyncHandle { #if defined(linux) SyncDriverErr -LinuxDriver_Freeze(const char *userPaths, +LinuxDriver_Freeze(const GSList *userPaths, SyncDriverHandle *handle); SyncDriverErr -VmSync_Freeze(const char *userPaths, +VmSync_Freeze(const GSList *userPaths, SyncDriverHandle *handle); SyncDriverErr -NullDriver_Freeze(const char *userPaths, +NullDriver_Freeze(const GSList *userPaths, SyncDriverHandle *handle); #endif diff --git a/open-vm-tools/lib/syncDriver/syncDriverLinux.c b/open-vm-tools/lib/syncDriver/syncDriverLinux.c index 8f13dc66f..c7ce315f4 100644 --- a/open-vm-tools/lib/syncDriver/syncDriverLinux.c +++ b/open-vm-tools/lib/syncDriver/syncDriverLinux.c @@ -32,7 +32,6 @@ #include #include "debug.h" #include "dynbuf.h" -#include "strutil.h" #include "syncDriverInt.h" /* Out toolchain headers are somewhat outdated and don't define these. */ @@ -44,7 +43,7 @@ typedef struct LinuxDriver { SyncHandle driver; - size_t fdCnt; + ssize_t fdCnt; int *fds; } LinuxDriver; @@ -66,11 +65,14 @@ typedef struct LinuxDriver { static SyncDriverErr LinuxFiThaw(const SyncDriverHandle handle) { - size_t i; + ssize_t i; LinuxDriver *sync = (LinuxDriver *) handle; SyncDriverErr err = SD_SUCCESS; - for (i = 0; i < sync->fdCnt; i++) { + /* + * Thaw in the reverse order of freeze + */ + for (i = sync->fdCnt - 1; i >= 0; i--) { if (ioctl(sync->fds[i], FITHAW) == -1) { err = SD_ERROR; } @@ -96,9 +98,12 @@ static void LinuxFiClose(SyncDriverHandle handle) { LinuxDriver *sync = (LinuxDriver *) handle; - size_t i; + ssize_t i; - for (i = 0; i < sync->fdCnt; i++) { + /* + * Close in the reverse order of open + */ + for (i = sync->fdCnt - 1; i >= 0; i--) { close(sync->fds[i]); } free(sync->fds); @@ -120,7 +125,7 @@ LinuxFiClose(SyncDriverHandle handle) * slow when guest is performing significant IO. Therefore, caller should * consider running this function in a separate thread. * - * @param[in] paths Paths to freeze (colon-separated). + * @param[in] paths List of paths to freeze. * @param[out] handle Handle to use for thawing. * * @return A SyncDriverErr. @@ -129,13 +134,10 @@ LinuxFiClose(SyncDriverHandle handle) */ SyncDriverErr -LinuxDriver_Freeze(const char *paths, +LinuxDriver_Freeze(const GSList *paths, SyncDriverHandle *handle) { - char *path; - int fd; - size_t count = 0; - unsigned int index = 0; + ssize_t count = 0; Bool first = TRUE; DynBuf fds; LinuxDriver *sync = NULL; @@ -143,7 +145,7 @@ LinuxDriver_Freeze(const char *paths, DynBuf_Init(&fds); - Debug(LGPFX "Freezing %s using Linux ioctls...\n", paths); + Debug(LGPFX "Freezing using Linux ioctls...\n"); sync = calloc(1, sizeof *sync); if (sync == NULL) { @@ -153,13 +155,21 @@ LinuxDriver_Freeze(const char *paths, sync->driver.thaw = LinuxFiThaw; sync->driver.close = LinuxFiClose; + /* + * Ensure we did not get an empty list + */ + VERIFY(paths != NULL); + /* * Iterate through the requested paths. If we get an error for the first * path, and it's not EPERM, assume that the ioctls are not available in * the current kernel. */ - while ((path = StrUtil_GetNextToken(&index, paths, ":")) != NULL) { + while (paths != NULL) { + int fd; + const char *path = paths->data; Debug(LGPFX "opening path '%s'.\n", path); + paths = g_slist_next(paths); fd = open(path, O_RDONLY); if (fd == -1) { switch (errno) { @@ -169,7 +179,6 @@ LinuxDriver_Freeze(const char *paths, * as users with permission 700, so just ignore these. */ Debug(LGPFX "cannot access mounted directory '%s'.\n", path); - free(path); continue; case EIO: @@ -179,14 +188,12 @@ LinuxDriver_Freeze(const char *paths, * this should be enough. Just skip. */ Debug(LGPFX "I/O error reading directory '%s'.\n", path); - free(path); continue; default: Debug(LGPFX "failed to open '%s': %d (%s)\n", path, errno, strerror(errno)); err = SD_ERROR; - free(path); goto exit; } } @@ -211,7 +218,6 @@ LinuxDriver_Freeze(const char *paths, Debug(LGPFX "failed to freeze '%s': %d (%s)\n", path, ioctlerr, strerror(ioctlerr)); err = first && ioctlerr == ENOTTY ? SD_UNAVAILABLE : SD_ERROR; - free(path); break; } } else { @@ -221,7 +227,6 @@ LinuxDriver_Freeze(const char *paths, Warning(LGPFX "failed to thaw '%s': %d (%s)\n", path, errno, strerror(errno)); } - free(path); close(fd); err = SD_ERROR; break; @@ -229,7 +234,6 @@ LinuxDriver_Freeze(const char *paths, count++; } - free(path); first = FALSE; } diff --git a/open-vm-tools/lib/syncDriver/syncDriverPosix.c b/open-vm-tools/lib/syncDriver/syncDriverPosix.c index 7cda4bbb5..f499faa47 100644 --- a/open-vm-tools/lib/syncDriver/syncDriverPosix.c +++ b/open-vm-tools/lib/syncDriver/syncDriverPosix.c @@ -25,9 +25,9 @@ #include #include #include +#include #include "vmware.h" #include "debug.h" -#include "dynbuf.h" #include "str.h" #include "syncDriverInt.h" #include "util.h" @@ -85,19 +85,21 @@ SyncDriverIsRemoteFSType(const char *fsType) /* *----------------------------------------------------------------------------- * - * SyncDriverListMounts -- + * SyncDriverLocalMounts -- * - * Returns a newly allocated string containing a list of colon-separated - * mount paths in the system. No filtering is done, so all paths are added. - * This assumes that the driver allows "unfreezable" paths to be provided - * to the freeze command. + * Returns a singly-linked list of all local disk paths mounted in the + * system filtering out remote file systems. There is no filtering for + * other mount points because we assume that the underlying driver and + * IOCTL can deal with "unfreezable" paths. The returned list of paths + * is in the reverse order of the paths returned by GETNEXT_MNTINFO. + * Caller must free each path and the list itself. * * XXX: mntinfo.h mentions Solaris and Linux, but not FreeBSD. If we ever * have a FreeBSD sync driver, we should make sure this function also * works there. * * Results: - * The list of devices to freeze, or NULL on failure. + * GSList* on success, NULL on failure. * * Side effects: * None @@ -105,21 +107,20 @@ SyncDriverIsRemoteFSType(const char *fsType) *----------------------------------------------------------------------------- */ -static char * -SyncDriverListMounts(void) +static GSList * +SyncDriverLocalMounts(void) { - char *paths = NULL; - DynBuf buf; + GSList *paths = NULL; MNTHANDLE mounts; DECLARE_MNTINFO(mntinfo); if ((mounts = OPEN_MNTFILE("r")) == NULL) { + Warning(LGPFX "Failed to open mount point table.\n"); return NULL; } - DynBuf_Init(&buf); - while (GETNEXT_MNTINFO(mounts, mntinfo)) { + char *path; /* * Skip remote mounts because they are not freezable and opening them * could lead to hangs. See PR 1196785. @@ -130,29 +131,21 @@ SyncDriverListMounts(void) continue; } + path = Util_SafeStrdup(MNTINFO_MNTPT(mntinfo)); + /* - * Add a separator if it's not the first path, and add the path to the - * tail of the list. + * A mount point could depend on existence of a previous mount + * point like a loopback. In order to avoid deadlock/hang in + * freeze operation, a mount point needs to be frozen before + * its dependency is frozen. + * Typically, mount points are listed in the order they are + * mounted by the system i.e. dependent comes after the + * dependency. So, we need to keep them in reverse order of + * mount points to achieve the dependency order. */ - if ((DynBuf_GetSize(&buf) != 0 && !DynBuf_Append(&buf, ":", 1)) - || !DynBuf_Append(&buf, - MNTINFO_MNTPT(mntinfo), - strlen(MNTINFO_MNTPT(mntinfo)))) { - goto exit; - } - } - - if (!DynBuf_Append(&buf, "\0", 1)) { - goto exit; + paths = g_slist_prepend(paths, path); } - paths = DynBuf_AllocGet(&buf); - if (paths == NULL) { - Debug(LGPFX "Failed to allocate path list.\n"); - } - -exit: - DynBuf_Destroy(&buf); (void) CLOSE_MNTFILE(mounts); return paths; } @@ -182,6 +175,29 @@ SyncDriver_Init(void) } +/* + *----------------------------------------------------------------------------- + * + * SyncDriverFreePath -- + * + * A GFunc for freeing path strings. It is intended for g_slist_foreach. + * + * Results: + * None + * + * Side effects: + * None + * + *----------------------------------------------------------------------------- + */ + +static void +SyncDriverFreePath(gpointer data, gpointer userData) +{ + free(data); +} + + /* *----------------------------------------------------------------------------- * @@ -214,14 +230,11 @@ SyncDriver_Freeze(const char *userPaths, // IN Bool enableNullDriver, // IN SyncDriverHandle *handle) // OUT { - char *paths = NULL; + GSList *paths = NULL; SyncDriverErr err = SD_UNAVAILABLE; size_t i = 0; /* - * First, convert the given path list to something the backends will - * understand: a colon-separated list of paths. - * * NOTE: Ignore disk UUIDs. We ignore the userPaths if it does * not start with '/' because all paths are absolute and it is * possible only when we get diskUUID as userPaths. So, all @@ -229,26 +242,42 @@ SyncDriver_Freeze(const char *userPaths, // IN */ if (userPaths == NULL || Str_Strncmp(userPaths, "all", sizeof "all") == 0 || - *userPaths != '/') { - paths = SyncDriverListMounts(); - if (paths == NULL) { - Debug(LGPFX "Failed to list mount points.\n"); - return SD_ERROR; - } + userPaths[0] != '/') { + paths = SyncDriverLocalMounts(); } else { /* - * The sync driver API specifies spaces as separators, but the driver - * uses colons as the path separator on Unix. + * The sync driver API specifies spaces as separators. */ - char *c; - paths = Util_SafeStrdup(userPaths); - for (c = paths; *c != '\0'; c++) { - if (*c == ' ') { - *c = ':'; + while (*userPaths != '\0') { + const char *c; + char *path; + + if (*userPaths == ' ') { + /* + * Trim spaces from beginning + */ + userPaths++; + continue; + } + + c = strchr(userPaths, ' '); + if (c == NULL) { + path = Util_SafeStrdup(userPaths); + paths = g_slist_append(paths, path); + break; + } else { + path = Util_SafeStrndup(userPaths, c - userPaths); + paths = g_slist_append(paths, path); + userPaths = c; } } } + if (paths == NULL) { + Warning(LGPFX "No paths to freeze.\n"); + return SD_ERROR; + } + while (err == SD_UNAVAILABLE && i < ARRAYSIZE(gBackends)) { SyncFreezeFn freezeFn = gBackends[i]; Debug(LGPFX "Calling backend %d.\n", (int) i); @@ -262,7 +291,12 @@ SyncDriver_Freeze(const char *userPaths, // IN err = freezeFn(paths, handle); } - free(paths); + /* + * g_slist_free_full requires glib >= v2.28 + */ + g_slist_foreach(paths, SyncDriverFreePath, NULL); + g_slist_free(paths); + return err == SD_SUCCESS; } diff --git a/open-vm-tools/lib/syncDriver/vmSyncDriver.c b/open-vm-tools/lib/syncDriver/vmSyncDriver.c index ea55d5796..2bd0e8862 100644 --- a/open-vm-tools/lib/syncDriver/vmSyncDriver.c +++ b/open-vm-tools/lib/syncDriver/vmSyncDriver.c @@ -24,9 +24,11 @@ #include #include +#include #include "debug.h" #include "syncDriverInt.h" #include "syncDriverIoc.h" +#include "strutil.h" #include "util.h" #define SYNC_PROC_PATH "/proc/driver/vmware-sync" @@ -89,7 +91,7 @@ VmSyncClose(SyncDriverHandle handle) * Opens a description to the driver's proc node, and if successful, send an * ioctl to freeze the requested filesystems. * - * @param[in] paths Paths to freeze (colon-delimited). + * @param[in] paths List of paths to freeze. * @param[out] handle Where to store the handle to use for thawing. * * @return A SyncDriverErr. @@ -98,19 +100,41 @@ VmSyncClose(SyncDriverHandle handle) */ SyncDriverErr -VmSync_Freeze(const char *paths, +VmSync_Freeze(const GSList *paths, SyncDriverHandle *handle) { int file; + Bool first = TRUE; + char *allPaths = NULL; VmSyncDriver *sync = NULL; - Debug(LGPFX "Freezing %s using vmsync driver...\n", paths); + Debug(LGPFX "Freezing using vmsync driver...\n"); file = open(SYNC_PROC_PATH, O_RDONLY); if (file == -1) { return SD_UNAVAILABLE; } + /* + * Ensure we did not get an empty list + */ + VERIFY(paths != NULL); + + /* + * Concatenate all paths in the list into one string + */ + while (paths != NULL) { + if (!first) { + /* + * Append the separator (':') + */ + StrUtil_SafeStrcat(&allPaths, ":"); + } + StrUtil_SafeStrcat(&allPaths, paths->data); + first = FALSE; + paths = g_slist_next(paths); + } + sync = calloc(1, sizeof *sync); if (sync == NULL) { goto exit; @@ -120,7 +144,9 @@ VmSync_Freeze(const char *paths, sync->driver.close = VmSyncClose; sync->fd = file; - if (ioctl(file, SYNC_IOC_FREEZE, paths) == -1) { + Debug(LGPFX "Freezing %s using vmsync driver...\n", allPaths); + + if (ioctl(file, SYNC_IOC_FREEZE, allPaths) == -1) { free(sync); sync = NULL; } @@ -133,6 +159,7 @@ exit: } else { *handle = &sync->driver; } + free(allPaths); return sync != NULL ? SD_SUCCESS : SD_ERROR; }