Re: [libvirt] [PATCH] Allow saving QEMU libvirt state to a pipe

On Fri, Nov 04, 2016 at 15:20:44 -0500, Roy Keene wrote: > All, > > Currently the "virsh save" command opens a file to save a domain's > XML and memory state, does the save, and then re-opens the file to > simulate seeking to the beginning to update the header to indicate that > the file is complete. > > 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. > > Attached is a patch that detects if the file being written to is not > seekable, and if so writes the completed header initially, then writes > the normal data, and avoids re-opening the file. > > This is useful to me for saving a VM state directly to Ceph RBD images > without having an intermediate file. > > I've tested this patch successfully (against v1.3.5) for both saving and > restoring a domain: > > # *( fifo="$(mktemp -u)"; mkfifo "${fifo}" && virsh save one-0 "${fifo}" > & cat "${fifo}" | rbd import - rbd/test1234 & wait; rm -f "${fifo}" )* > > Domain one-0 saved to /tmp/tmp.HK4hChiQqB > > # > > # *( fifo="$(mktemp -u)"; mkfifo "${fifo}" && rbd export rbd/test1234 - > > "${fifo}" & virsh restore "${fifo}" & wait; rm -f "${fifo}" ) * > Exporting image: 100% complete...done. > Domain restored from /tmp/tmp.0YaUZ5Y2yT > > # *virsh list* > Id Name State > ---------------------------------------------------- > 11 one-0 running > # > > -- > Roy Keene > Knight Point Systems, LLC > Service-Disabled Veteran-Owned Business > 1775 Wiehle Avenue Suite 101 | Reston, VA 20190 > c: 813-956-3808 f: 571-266-3106 > www.knightpoint.com > DHS EAGLE II Prime Contractor: FC1 SDVOSB Track > GSA Schedule 70 SDVOSB: GS-35F-0646S > GSA MOBIS Schedule: GS-10F-0404Y > ISO 20000 / ISO 27001 > > Notice: This e-mail message, including any attachments, is for the > sole use of the intended recipient(s) and may contain confidential > and privileged information. Any unauthorized review, copy, use, > disclosure, or distribution is STRICTLY prohibited. If you are not > the intended recipient, please contact the sender by reply e-mail > and destroy all copies of the original message. Trimming this on public postings might be a good idea.

> diff --no-dereference -uNr libvirt-1.3.5.orig/src/qemu/qemu_driver.c libvirt-1.3.5-savepipe/src/qemu/qemu_driver.c > --- libvirt-1.3.5.orig/src/qemu/qemu_driver.c 2016-05-28 11:47:33.000000000 -0500 > +++ libvirt-1.3.5-savepipe/src/qemu/qemu_driver.c 2016-11-04 10:54:15.160805261 -0500 This is a very old version of libvirt.

Please rebase the changes against the git master since they can't be applied: Applying: Allow saving QEMU libvirt state to a pipe error: patch failed: src/qemu/qemu_driver.c:3124 error: src/qemu/qemu_driver.c: patch does not apply Patch failed at 0001 Allow saving QEMU libvirt state to a pipe Additionally please generate the patches using git so that we have a proper commit message.

> @@ -3080,6 +3080,7 @@ > virQEMUSaveHeader header; > bool bypassSecurityDriver = false; > bool needUnlink = false; > + bool canReopen = true; > int ret = -1; > int fd = -1; > int directFlag = 0; > @@ -3087,7 +3088,6 @@ > unsigned int wrapperFlags = VIR_FILE_WRAPPER_NON_BLOCKING; > > 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; > @@ -3109,12 +3109,32 @@ > if (fd < 0) > goto cleanup; > > + /* > + * Determine if this file can be re-opened > + * > + * Right now we just try to seek into it, if that fails then that means we > + * are probably not dealing with a regular file here and we probably > + * cannot reopen it. > + */ Another option would be to pass a flag to the API, where the user would declare by using such flag that the output is valid only on success of the command and thus rewriting the header is not necessary.

> + if (lseek(fd, 0, SEEK_SET) == ((off_t) -1)) { > + canReopen = false; > + } This will fail make syntax-check on current master since single line if statement bodies should not be put into braces.

> + > if (virSecurityManagerSetImageFDLabel(driver->securityManager, vm->def, fd) < 0) > goto cleanup; > > if (!(wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags))) > goto cleanup; > > + /* Set the header magic */ > + if (canReopen) { > + /* We will update the magic after the saving completes successfully */ > + memcpy(header.magic, QEMU_SAVE_PARTIAL, sizeof(header.magic)); > + } else { > + /* If we cannot re-open the output, include the final magic here */ > + memcpy(header.magic, QEMU_SAVE_MAGIC, sizeof(header.magic)); This is okay though, comments are counted as a line too. > + } > + > /* Write header to file, followed by XML */ > if (qemuDomainSaveHeader(fd, path, domXML, &header) < 0) > goto cleanup; The rest looks reasonable. I'd slightly more prefer the use of the flag in comparison to auto detection.

Thanks for your patch thoug. Peter