[pardon the top post] Doing some (personal) mail box cleaning and found this "older" patch... Sorry that this has sat unattended (and probably now forgotten or given up on) for so long. I don't even recall v1 or v2 reviews - been too long. In any case, if you still would like to see this addressed, could you rebase on top of tree and send again along with a few points below... On 03/08/2017 11:22 PM, Chen Hanxiao wrote:

From: Chen Hanxiao <chenhanxiao@gmail.com>

Base upon patches from Roy Keene <rkeene@knightpoint.com>

Currently qemuDomainSaveMemory can save vm's config and memory to fd. It write a magic QEMU_SAVE_PARTIAL firstly, then re-open it to change QEMU_SAVE_PARTIAL as QEMU_SAVE_MAGIC.

For pipes this is not possible, attempting to re-open the pipe will not connect you to the same consumer. Seeking is also not possible on a pipe.

This patch introduce VIR_DOMAIN_SAVE_DIRECT If set, write QEMU_SAVE_MAGIC directly.

This is useful to me for saving a VM state directly to Ceph RBD images without having an intermediate file.

The above really needs to be cleaned up to be less choppy. I see some of this is copied later too

Signed-off-by: Roy Keene <rkeene@knightpoint.com> Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com> --- v3: add news.xml

v2-resend: rebase on upstream

v2: rename VIR_DOMAIN_SAVE_PIPE to VIR_DOMAIN_SAVE_DIRECT remove S_ISFIFO check for dst path

docs/news.xml | 9 +++++++ include/libvirt/libvirt-domain.h | 1 + src/qemu/qemu_driver.c | 54 ++++++++++++++++++++++++++-------------- 3 files changed, 46 insertions(+), 18 deletions(-)

You'll need to understand the changes in the code made more recently for commit ids 'a2d2aae14' and 'ec986bc5' which altered the flow of the code to make things more common... I had to go all the way back to Dec archives to find any comment regarding this series: https://www.redhat.com/archives/libvir-list/2016-December/msg00318.html and before that Roy's series: https://www.redhat.com/archives/libvir-list/2016-November/msg00269.html So while I know it's important to have some continuity for some reasons vis-a-vis the subject line; however, at this point I think you just need to make a fresh subject indicating the ability to save 'directly'. Using a pipe just happens to be an implementation of that concept. You can and should include pointers to the archives of the previous series. It helps set context for the reviewer when a v4 arrives in which they don't recognize the subject.

diff --git a/docs/news.xml b/docs/news.xml index 9515395..57088db 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -37,6 +37,15 @@ applications running on the platform. </description> </change> + <change> + <summary> + qemu: Allow qemuDomainSaveMemory saving VM state to a pipe> + </summary> + <description> + Introduce flag VIR_DOMAIN_SAVE_DIRECT to enable command 'save' + to write to PIPE, for PIPE can't be reopened. + </description> + </change> </section> <section title="Bug fixes"> <change/>

This should be its own separate third patch. Still you should reword to avoid directly mentioning pipe or fifo, but using directly to some already opened target. Perhaps in the description you can mention that by directly, this means you could use a pipe and provide that example (but try to do so without ceph). IOW: How would someone use this in a general sense. Would some sort of socket be another way to use this? The difference w/ direct is that the to be saved "save-image" destination doesn't necessarily need to be to a file, it can be to something else via a pipe or some other indirection, right? (IIUC at least)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index c490d71..f58fe2c 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1169,6 +1169,7 @@ typedef enum { VIR_DOMAIN_SAVE_BYPASS_CACHE = 1 << 0, /* Avoid file system cache pollution */ VIR_DOMAIN_SAVE_RUNNING = 1 << 1, /* Favor running over paused */ VIR_DOMAIN_SAVE_PAUSED = 1 << 2, /* Favor paused over running */ + VIR_DOMAIN_SAVE_DIRECT = 1 << 3, /* Write QEMU_SAVE_MAGIC directly */

I'd probably avoid mentioning QEMU_SAVE_MAGIC...

} virDomainSaveRestoreFlags;

int virDomainSave (virDomainPtr domain, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2032fac..29b7677 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3059,6 +3059,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver, virQEMUSaveHeader header; bool bypassSecurityDriver = false; bool needUnlink = false; + bool canReopen = true;

I think I would have gone with something like: bool isDirect = (flags & VIR_DOMAIN_SAVE_DIRECT);

int ret = -1; int fd = -1; int directFlag = 0; @@ -3066,7 +3067,6 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver, unsigned int wrapperFlags = VIR_FILE_WRAPPER_NON_BLOCKING;

hmmm... NON_BLOCKING - that's triggering something I looked at recently... But how would --bypass-cache affect things for --direct?

memset(&header, 0, sizeof(header)); - memcpy(header.magic, QEMU_SAVE_PARTIAL, sizeof(header.magic)); header.version = QEMU_SAVE_VERSION; header.was_running = was_running ? 1 : 0; header.compressed = compressed; @@ -3082,6 +3082,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver, goto cleanup; } } +

Unrelated.

fd = qemuOpenFile(driver, vm, path, O_WRONLY | O_TRUNC | O_CREAT | directFlag, &needUnlink, &bypassSecurityDriver); @@ -3094,6 +3095,20 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver, if (!(wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags))) goto cleanup;

+ /* Set the header magic. + * Setting flags VIR_DOMAIN_SAVE_DIRECT will write + * magic QEMU_SAVE_MAGIC directly. + * For PIPE, we should do this because it can't be reopen. + * Otherwise we'll update the magic after + * the saving completes successfully. + */

The comment is "choppy", but I pointed that out above.

+ if (flags & VIR_DOMAIN_SAVE_DIRECT) {

if (isDirect) memcpy MAGIC else mempcy PARTIAL of course this code changed, so you'll need to see how you're affected.

+ canReopen = false; + memcpy(header.magic, QEMU_SAVE_MAGIC, sizeof(header.magic)); + } else { + memcpy(header.magic, QEMU_SAVE_PARTIAL, sizeof(header.magic)); + } + /* Write header to file, followed by XML */ if (qemuDomainSaveHeader(fd, path, domXML, &header) < 0) goto cleanup; @@ -3102,28 +3117,30 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver, if (qemuMigrationToFile(driver, vm, fd, compressedpath, asyncJob) < 0) goto cleanup;

Then add a check here: if (isDirect) { ret = 0; goto cleanup; } Leaving the next pile untouched

- /* Touch up file header to mark image complete. */ + if (canReopen) { + /* Touch up file header to mark image complete. */

- /* Reopen the file to touch up the header, since we aren't set - * up to seek backwards on wrapperFd. The reopened fd will - * trigger a single page of file system cache pollution, but - * that's acceptable. */ - if (VIR_CLOSE(fd) < 0) { - virReportSystemError(errno, _("unable to close %s"), path); - goto cleanup; - } + /* Reopen the file to touch up the header, since we aren't set + * up to seek backwards on wrapperFd. The reopened fd will + * trigger a single page of file system cache pollution, but + * that's acceptable. */

Notice how your comments didn't line up here. Won't matter if you use the goto I suggest above. Still I wonder about the viability of using a pipe for something sufficiently large or something that gets interrupted. Could having the QEMU_SAVE_MAGIC in the header cause "some other" mechanism to believe it has a completely written file? IOW: How does something else know it's done? Does that matter? Perhaps not for the case you're using, but in a more general sense. I don't know the save/restore code all that well, but I know save does use iohelper and IIUC the whole purpose of PARTIAL and MAGIC is to somehow indicate to a different operation the state of the 'file' being saved. I responded recently to someone having a patch to use iohelper on restore, not sure if what I wrote there will help or not, but it does have some pointers to patches for iohelper/cache processing, see: https://www.redhat.com/archives/libvir-list/2017-July/msg00176.html Again save/restore processing is not something I've studied that closely, but my main concern is there are two known states - MAGIC (complete) and PARTIAL (incomplete) that other code may make some assumptions about that you'd be circumventing by immediately setting MAGIC (done). I may be off base, but I figured I'd at least mention it to make sure it's felt it's not a problem in this instance. John

+ if (VIR_CLOSE(fd) < 0) { + virReportSystemError(errno, _("unable to close %s"), path); + goto cleanup; + }

- if (virFileWrapperFdClose(wrapperFd) < 0) - goto cleanup; + if (virFileWrapperFdClose(wrapperFd) < 0) + goto cleanup;

- if ((fd = qemuOpenFile(driver, vm, path, O_WRONLY, NULL, NULL)) < 0) - goto cleanup; + if ((fd = qemuOpenFile(driver, vm, path, O_WRONLY, NULL, NULL)) < 0) + goto cleanup;

- memcpy(header.magic, QEMU_SAVE_MAGIC, sizeof(header.magic)); + memcpy(header.magic, QEMU_SAVE_MAGIC, sizeof(header.magic));

- if (safewrite(fd, &header, sizeof(header)) != sizeof(header)) { - virReportSystemError(errno, _("unable to write %s"), path); - goto cleanup; + if (safewrite(fd, &header, sizeof(header)) != sizeof(header)) { + virReportSystemError(errno, _("unable to write %s"), path); + goto cleanup; + } }

if (VIR_CLOSE(fd) < 0) { @@ -3353,6 +3370,7 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml,

virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE | VIR_DOMAIN_SAVE_RUNNING | + VIR_DOMAIN_SAVE_DIRECT | VIR_DOMAIN_SAVE_PAUSED, -1);

cfg = virQEMUDriverGetConfig(driver);

On 03/08/2017 11:22 PM, Chen Hanxiao wrote:

From: Chen Hanxiao <chenhanxiao@gmail.com>

Base upon patches from Roy Keene <rkeene@knightpoint.com>

This patch introduces --direct flag for save command.

We could use this flag to save vm to a PIPE.

We could saving a VM state directly to Ceph RBD images without having an intermediate file.

How to test: fifo="$(mktemp -u)"; mkfifo "${fifo}" && virsh save --pipe cirros "${fifo}" & cat "${fifo}" | rbd --id cinder import - hotsnapshot/test1234 & wait; rm -f "${fifo}"

Is there a way to do this without rbd at the other end? Is there another example to provide... Perhaps something that would be documented else where was well rather than just in the bowels of virsh. Perhaps somewhere where the other VIR_DOMAIN_SAVE_* flags are described.

Signed-off-by: Roy Keene <rkeene@knightpoint.com> Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com> --- v3: rebase on upstream

v2-resend: rebase on upstream

v2: rename VIR_DOMAIN_SAVE_PIPE to VIR_DOMAIN_SAVE_DIRECT

tools/virsh-domain.c | 6 ++++++ tools/virsh.pod | 5 ++++- 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 09a9f82..d96e894 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -4192,6 +4192,10 @@ static const vshCmdOptDef opts_save[] = { .type = VSH_OT_BOOL, .help = N_("set domain to be paused on restore") }, + {.name = "direct", + .type = VSH_OT_BOOL, + .help = N_("write the file directly, needed by PIPE/FIFO")

remove ", needed by PIPE/FIFO"

+ }, {.name = "verbose", .type = VSH_OT_BOOL, .help = N_("display the progress of save") @@ -4228,6 +4232,8 @@ doSave(void *opaque) flags |= VIR_DOMAIN_SAVE_RUNNING; if (vshCommandOptBool(cmd, "paused")) flags |= VIR_DOMAIN_SAVE_PAUSED; + if (vshCommandOptBool(cmd, "direct")) + flags |= VIR_DOMAIN_SAVE_DIRECT;

if (vshCommandOptStringReq(ctl, cmd, "xml", &xmlfile) < 0) goto out; diff --git a/tools/virsh.pod b/tools/virsh.pod index ee79046..9dcb527 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1928,7 +1928,7 @@ have also reverted all storage volumes back to the same contents as when the state file was created.

=item B<save> I<domain> I<state-file> [I<--bypass-cache>] [I<--xml> B<file>] -[{I<--running> | I<--paused>}] [I<--verbose>] +[{I<--running> | I<--paused>}] [I<--direct>] [I<--verbose>]

Saves a running domain (RAM, but not disk state) to a state file so that it can be restored @@ -1943,6 +1943,9 @@ with B<domjobabort> command (sent by another virsh instance). Another option is to send SIGINT (usually with C<Ctrl-C>) to the virsh process running B<save> command. I<--verbose> displays the progress of save.

+Usually B<save> command will save the domain's state as a regular file. +If you want to save it into a PIPE/FIFO, then flag I<--direct> must be set. +

As noted in review of .1 - how does --bypass-cache affect things usage wise? Does it matter, is it helpful. I wish I could think of a better way to say this right now, but it's late so I'm just winging it. If there's a way to say generically what direct does where "pipe" is a possible mechanism to use (as is I assume a socket). John

This is roughly equivalent to doing a hibernate on a running computer, with all the same limitations. Open network connections may be severed upon restore, as TCP timeouts may have expired.