Fix QEMU save/restore with block devices

The save process was relying on use of the shell >> append
operator to ensure the save data was placed after the libvirt
header + XML. This doesn't work for block devices though.
Replace this code with use of 'dd' and its 'seek' parameter.
This means that we need to pad the header + XML out to a
multiple of dd block size (in this case we choose 512).

The qemuMonitorMigateToCommand() monitor API is used for both
save/coredump, and migration via UNIX socket. We can't simply
switch this to use 'dd' since this causes problems with the
migration usage. Thus, create a dedicated qemuMonitorMigateToFile
which can accept an filename + offset, and remove the filename
from the current qemuMonitorMigateToCommand() API

* src/qemu/qemu_driver.c: Switch to qemuMonitorMigateToFile
  for save and core dump
* src/qemu/qemu_monitor.c, src/qemu/qemu_monitor.h,
  src/qemu/qemu_monitor_json.c, src/qemu/qemu_monitor_json.h,
  src/qemu/qemu_monitor_text.c, src/qemu/qemu_monitor_text.h: Create
  a new qemuMonitorMigateToFile, separate from the existing
  qemuMonitorMigateToCommand to allow handling file offsets

7 files changed, 232 insertions, 96 deletions

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c485e8fb8..19246fd1a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4715,6 +4715,7 @@ static int qemudDomainSaveFlag(virDomainPtr dom, const char *path,
     qemuDomainObjPrivatePtr priv;
     struct stat sb;
     int is_reg = 0;
+    unsigned long long offset;
 
     memset(&header, 0, sizeof(header));
     memcpy(header.magic, QEMUD_SAVE_MAGIC, sizeof(header.magic));
@@ -4788,104 +4789,137 @@ static int qemudDomainSaveFlag(virDomainPtr dom, const char *path,
     hdata.path = path;
     hdata.xml = xml;
     hdata.header = &header;
+    offset = sizeof(header) + header.xml_len;
+
+    /* Due to way we append QEMU state on our header with dd,
+     * we need to ensure there's a 512 byte boundary. Unfortunately
+     * we don't have an explicit offset in the header, so we fake
+     * it by padding the XML string with NULLs */
+    if (offset % QEMU_MONITOR_MIGRATE_TO_FILE_BS) {
+        unsigned long long pad =
+            QEMU_MONITOR_MIGRATE_TO_FILE_BS -
+            (offset % QEMU_MONITOR_MIGRATE_TO_FILE_BS);
+
+        if (VIR_REALLOC_N(xml, header.xml_len + pad) < 0) {
+            virReportOOMError();
+            goto endjob;
+        }
+        memset(xml + header.xml_len, 0, pad);
+        offset += pad;
+        header.xml_len += pad;
+    }
 
     /* Write header to file, followed by XML */
 
     /* First try creating the file as root */
-    if (is_reg &&
-        (rc = virFileOperation(path, O_CREAT|O_TRUNC|O_WRONLY,
-                               S_IRUSR|S_IWUSR,
-                               getuid(), getgid(),
-                               qemudDomainSaveFileOpHook, &hdata,
-                               0)) != 0) {
-
-        /* If we failed as root, and the error was permission-denied
-           (EACCES), assume it's on a network-connected share where
-           root access is restricted (eg, root-squashed NFS). If the
-           qemu user (driver->user) is non-root, just set a flag to
-           bypass security driver shenanigans, and retry the operation
-           after doing setuid to qemu user */
-
-        if ((rc != EACCES) ||
-            driver->user == getuid()) {
-            virReportSystemError(rc, _("Failed to create domain save file '%s'"),
-                                 path);
+    if (!is_reg) {
+        int fd = open(path, O_WRONLY | O_TRUNC);
+        if (fd < 0) {
+            virReportSystemError(errno, _("unable to open %s"), path);
             goto endjob;
         }
-
-#ifdef __linux__
-        /* On Linux we can also verify the FS-type of the directory. */
-        char *dirpath, *p;
-        struct statfs st;
-        int statfs_ret;
-
-        if ((dirpath = strdup(path)) == NULL) {
-            virReportOOMError();
+        if ((rc = qemudDomainSaveFileOpHook(fd, &hdata)) != 0) {
+            close(fd);
+            goto endjob;
+        }
+        if (close(fd) < 0) {
+            virReportSystemError(errno, _("unable to close %s"), path);
             goto endjob;
         }
+    } else {
+        if ((rc = virFileOperation(path, O_CREAT|O_TRUNC|O_WRONLY,
+                                  S_IRUSR|S_IWUSR,
+                                  getuid(), getgid(),
+                                  qemudDomainSaveFileOpHook, &hdata,
+                                  0)) != 0) {
+            /* If we failed as root, and the error was permission-denied
+               (EACCES), assume it's on a network-connected share where
+               root access is restricted (eg, root-squashed NFS). If the
+               qemu user (driver->user) is non-root, just set a flag to
+               bypass security driver shenanigans, and retry the operation
+               after doing setuid to qemu user */
+
+            if ((rc != EACCES) ||
+                driver->user == getuid()) {
+                virReportSystemError(rc, _("Failed to create domain save file '%s'"),
+                                     path);
+                goto endjob;
+            }
 
-        do {
-            // Try less and less of the path until we get to a
-            // directory we can stat. Even if we don't have 'x'
-            // permission on any directory in the path on the NFS
-            // server (assuming it's NFS), we will be able to stat the
-            // mount point, and that will properly tell us if the
-            // fstype is NFS.
+#ifdef __linux__
+            /* On Linux we can also verify the FS-type of the directory. */
+            char *dirpath, *p;
+            struct statfs st;
+            int statfs_ret;
 
-            if ((p = strrchr(dirpath, '/')) == NULL) {
-                qemuReportError(VIR_ERR_INVALID_ARG,
-                                _("Invalid relative path '%s' for domain save file"),
-                                path);
-                VIR_FREE(dirpath);
+            if ((dirpath = strdup(path)) == NULL) {
+                virReportOOMError();
                 goto endjob;
             }
 
-            if (p == dirpath)
-                *(p+1) = '\0';
-            else
-                *p = '\0';
+            do {
+                // Try less and less of the path until we get to a
+                // directory we can stat. Even if we don't have 'x'
+                // permission on any directory in the path on the NFS
+                // server (assuming it's NFS), we will be able to stat the
+                // mount point, and that will properly tell us if the
+                // fstype is NFS.
+
+                if ((p = strrchr(dirpath, '/')) == NULL) {
+                    qemuReportError(VIR_ERR_INVALID_ARG,
+                                    _("Invalid relative path '%s' for domain save file"),
+                                    path);
+                    VIR_FREE(dirpath);
+                    goto endjob;
+                }
 
-            statfs_ret = statfs(dirpath, &st);
+                if (p == dirpath)
+                    *(p+1) = '\0';
+                else
+                    *p = '\0';
 
-        } while ((statfs_ret == -1) && (p != dirpath));
+                statfs_ret = statfs(dirpath, &st);
 
-        if (statfs_ret == -1) {
-            virReportSystemError(errno,
-                                 _("Failed to create domain save file '%s'"
-                                   " statfs of all elements of path failed."),
-                                 path);
-            VIR_FREE(dirpath);
-            goto endjob;
-        }
+            } while ((statfs_ret == -1) && (p != dirpath));
 
-        if (st.f_type != NFS_SUPER_MAGIC) {
-            virReportSystemError(rc,
-                                 _("Failed to create domain save file '%s'"
-                                   " (fstype of '%s' is 0x%X"),
-                                 path, dirpath, (unsigned int) st.f_type);
+            if (statfs_ret == -1) {
+                virReportSystemError(errno,
+                                     _("Failed to create domain save file '%s'"
+                                       " statfs of all elements of path failed."),
+                                     path);
+                VIR_FREE(dirpath);
+                goto endjob;
+            }
+
+            if (st.f_type != NFS_SUPER_MAGIC) {
+                virReportSystemError(rc,
+                                     _("Failed to create domain save file '%s'"
+                                       " (fstype of '%s' is 0x%X"),
+                                     path, dirpath, (unsigned int) st.f_type);
+                VIR_FREE(dirpath);
+                goto endjob;
+            }
             VIR_FREE(dirpath);
-            goto endjob;
-        }
-        VIR_FREE(dirpath);
 #endif
 
-        /* Retry creating the file as driver->user */
+            /* Retry creating the file as driver->user */
 
-        if ((rc = virFileOperation(path, O_CREAT|O_TRUNC|O_WRONLY,
-                                   S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP,
-                                   driver->user, driver->group,
-                                   qemudDomainSaveFileOpHook, &hdata,
-                                   VIR_FILE_OP_AS_UID)) != 0) {
-            virReportSystemError(rc, _("Error from child process creating '%s'"),
+            if ((rc = virFileOperation(path, O_CREAT|O_TRUNC|O_WRONLY,
+                                       S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP,
+                                       driver->user, driver->group,
+                                       qemudDomainSaveFileOpHook, &hdata,
+                                       VIR_FILE_OP_AS_UID)) != 0) {
+                virReportSystemError(rc, _("Error from child process creating '%s'"),
                                  path);
-            goto endjob;
-        }
+                goto endjob;
+            }
 
-        /* Since we had to setuid to create the file, and the fstype
-           is NFS, we assume it's a root-squashing NFS share, and that
-           the security driver stuff would have failed anyway */
+            /* Since we had to setuid to create the file, and the fstype
+               is NFS, we assume it's a root-squashing NFS share, and that
+               the security driver stuff would have failed anyway */
 
-        bypassSecurityDriver = 1;
+            bypassSecurityDriver = 1;
+        }
     }
 
 
@@ -4898,7 +4932,7 @@ static int qemudDomainSaveFlag(virDomainPtr dom, const char *path,
     if (header.compressed == QEMUD_SAVE_FORMAT_RAW) {
         const char *args[] = { "cat", NULL };
         qemuDomainObjEnterMonitorWithDriver(driver, vm);
-        rc = qemuMonitorMigrateToCommand(priv->mon, 1, args, path);
+        rc = qemuMonitorMigrateToFile(priv->mon, 1, args, path, offset);
         qemuDomainObjExitMonitorWithDriver(driver, vm);
     } else {
         const char *prog = qemudSaveCompressionTypeToString(header.compressed);
@@ -4908,7 +4942,7 @@ static int qemudDomainSaveFlag(virDomainPtr dom, const char *path,
             NULL
         };
         qemuDomainObjEnterMonitorWithDriver(driver, vm);
-        rc = qemuMonitorMigrateToCommand(priv->mon, 1, args, path);
+        rc = qemuMonitorMigrateToFile(priv->mon, 1, args, path, offset);
         qemuDomainObjExitMonitorWithDriver(driver, vm);
     }
 
@@ -5203,7 +5237,7 @@ static int qemudDomainCoreDump(virDomainPtr dom,
     }
 
     qemuDomainObjEnterMonitorWithDriver(driver, vm);
-    ret = qemuMonitorMigrateToCommand(priv->mon, 1, args, path);
+    ret = qemuMonitorMigrateToFile(priv->mon, 1, args, path, 0);
     qemuDomainObjExitMonitorWithDriver(driver, vm);
 
     if (ret < 0)
@@ -9896,7 +9930,7 @@ static int doTunnelMigrate(virDomainPtr dom,
         internalret = qemuMonitorMigrateToUnix(priv->mon, 1, unixfile);
     else if (qemuCmdFlags & QEMUD_CMD_FLAG_MIGRATE_QEMU_EXEC) {
         const char *args[] = { "nc", "-U", unixfile, NULL };
-        internalret = qemuMonitorMigrateToCommand(priv->mon, 1, args, "/dev/null");
+        internalret = qemuMonitorMigrateToCommand(priv->mon, 1, args);
     } else {
         internalret = -1;
     }
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 38ee05835..5e83afc27 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1178,17 +1178,40 @@ int qemuMonitorMigrateToHost(qemuMonitorPtr mon,
 
 int qemuMonitorMigrateToCommand(qemuMonitorPtr mon,
                                 int background,
-                                const char * const *argv,
-                                const char *target)
+                                const char * const *argv)
 {
     int ret;
-    DEBUG("mon=%p, fd=%d argv=%p target=%s",
-          mon, mon->fd, argv, target);
+    DEBUG("mon=%p, fd=%d argv=%p",
+          mon, mon->fd, argv);
 
     if (mon->json)
-        ret = qemuMonitorJSONMigrateToCommand(mon, background, argv, target);
+        ret = qemuMonitorJSONMigrateToCommand(mon, background, argv);
     else
-        ret = qemuMonitorTextMigrateToCommand(mon, background, argv, target);
+        ret = qemuMonitorTextMigrateToCommand(mon, background, argv);
+    return ret;
+}
+
+int qemuMonitorMigrateToFile(qemuMonitorPtr mon,
+                             int background,
+                             const char * const *argv,
+                             const char *target,
+                             unsigned long long offset)
+{
+    int ret;
+    DEBUG("mon=%p, fd=%d argv=%p target=%s offset=%llu",
+          mon, mon->fd, argv, target, offset);
+
+    if (offset % QEMU_MONITOR_MIGRATE_TO_FILE_BS) {
+        qemuReportError(VIR_ERR_INTERNAL_ERROR,
+                        _("file offset must be a multiple of %llu"),
+                        QEMU_MONITOR_MIGRATE_TO_FILE_BS);
+        return -1;
+    }
+
+    if (mon->json)
+        ret = qemuMonitorJSONMigrateToFile(mon, background, argv, target, offset);
+    else
+        ret = qemuMonitorTextMigrateToFile(mon, background, argv, target, offset);
     return ret;
 }
 
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 251233a54..ec848a8ed 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -246,8 +246,15 @@ int qemuMonitorMigrateToHost(qemuMonitorPtr mon,
 
 int qemuMonitorMigrateToCommand(qemuMonitorPtr mon,
                                 int background,
-                                const char * const *argv,
-                                const char *target);
+                                const char * const *argv);
+
+# define QEMU_MONITOR_MIGRATE_TO_FILE_BS 512llu
+
+int qemuMonitorMigrateToFile(qemuMonitorPtr mon,
+                             int background,
+                             const char * const *argv,
+                             const char *target,
+                             unsigned long long offset);
 
 int qemuMonitorMigrateToUnix(qemuMonitorPtr mon,
                              int background,
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 7c8387d48..09a356217 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -1483,8 +1483,36 @@ int qemuMonitorJSONMigrateToHost(qemuMonitorPtr mon,
 
 int qemuMonitorJSONMigrateToCommand(qemuMonitorPtr mon,
                                     int background,
-                                    const char * const *argv,
-                                    const char *target)
+                                    const char * const *argv)
+{
+    char *argstr;
+    char *dest = NULL;
+    int ret = -1;
+
+    argstr = virArgvToString(argv);
+    if (!argstr) {
+        virReportOOMError();
+        goto cleanup;
+    }
+
+    if (virAsprintf(&dest, "exec:%s", argstr) < 0) {
+        virReportOOMError();
+        goto cleanup;
+    }
+
+    ret = qemuMonitorJSONMigrate(mon, background, dest);
+
+cleanup:
+    VIR_FREE(argstr);
+    VIR_FREE(dest);
+    return ret;
+}
+
+int qemuMonitorJSONMigrateToFile(qemuMonitorPtr mon,
+                                 int background,
+                                 const char * const *argv,
+                                 const char *target,
+                                 unsigned long long offset)
 {
     char *argstr;
     char *dest = NULL;
@@ -1504,7 +1532,10 @@ int qemuMonitorJSONMigrateToCommand(qemuMonitorPtr mon,
         goto cleanup;
     }
 
-    if (virAsprintf(&dest, "exec:%s >>%s 2>/dev/null", argstr, safe_target) < 0) {
+    if (virAsprintf(&dest, "exec:%s | dd of=%s bs=%llu seek=%llu",
+                    argstr, safe_target,
+                    QEMU_MONITOR_MIGRATE_TO_FILE_BS,
+                    offset / QEMU_MONITOR_MIGRATE_TO_FILE_BS) < 0) {
         virReportOOMError();
         goto cleanup;
     }
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index f404c561d..07fa587d3 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -100,8 +100,13 @@ int qemuMonitorJSONMigrateToHost(qemuMonitorPtr mon,
 
 int qemuMonitorJSONMigrateToCommand(qemuMonitorPtr mon,
                                     int background,
-                                    const char * const *argv,
-                                    const char *target);
+                                    const char * const *argv);
+
+int qemuMonitorJSONMigrateToFile(qemuMonitorPtr mon,
+                                 int background,
+                                 const char * const *argv,
+                                 const char *target,
+                                 unsigned long long offset);
 
 int qemuMonitorJSONMigrateToUnix(qemuMonitorPtr mon,
                                  int background,
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index 4c8c1083a..3f917bff8 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -1202,8 +1202,36 @@ int qemuMonitorTextMigrateToHost(qemuMonitorPtr mon,
 
 int qemuMonitorTextMigrateToCommand(qemuMonitorPtr mon,
                                     int background,
-                                    const char * const *argv,
-                                    const char *target)
+                                    const char * const *argv)
+{
+    char *argstr;
+    char *dest = NULL;
+    int ret = -1;
+
+    argstr = virArgvToString(argv);
+    if (!argstr) {
+        virReportOOMError();
+        goto cleanup;
+    }
+
+    if (virAsprintf(&dest, "exec:%s", argstr) < 0) {
+        virReportOOMError();
+        goto cleanup;
+    }
+
+    ret = qemuMonitorTextMigrate(mon, background, dest);
+
+cleanup:
+    VIR_FREE(argstr);
+    VIR_FREE(dest);
+    return ret;
+}
+
+int qemuMonitorTextMigrateToFile(qemuMonitorPtr mon,
+                                 int background,
+                                 const char * const *argv,
+                                 const char *target,
+                                 unsigned long long offset)
 {
     char *argstr;
     char *dest = NULL;
@@ -1223,7 +1251,10 @@ int qemuMonitorTextMigrateToCommand(qemuMonitorPtr mon,
         goto cleanup;
     }
 
-    if (virAsprintf(&dest, "exec:%s >>%s 2>/dev/null", argstr, safe_target) < 0) {
+    if (virAsprintf(&dest, "exec:%s | dd of=%s bs=%llu seek=%llu",
+                    argstr, safe_target,
+                    QEMU_MONITOR_MIGRATE_TO_FILE_BS,
+                    offset / QEMU_MONITOR_MIGRATE_TO_FILE_BS) < 0) {
         virReportOOMError();
         goto cleanup;
     }
diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h
index 3200660c4..23c3a4500 100644
--- a/src/qemu/qemu_monitor_text.h
+++ b/src/qemu/qemu_monitor_text.h
@@ -99,8 +99,13 @@ int qemuMonitorTextMigrateToHost(qemuMonitorPtr mon,
 
 int qemuMonitorTextMigrateToCommand(qemuMonitorPtr mon,
                                     int background,
-                                    const char * const *argv,
-                                    const char *target);
+                                    const char * const *argv);
+
+int qemuMonitorTextMigrateToFile(qemuMonitorPtr mon,
+                                 int background,
+                                 const char * const *argv,
+                                 const char *target,
+                                 unsigned long long offset);
 
 int qemuMonitorTextMigrateToUnix(qemuMonitorPtr mon,
                                  int background,