Re: [libvirt RFCv7 22/40] qemu: saveimage: convert qemuSaveImageOpen to use virQEMUSaveFd

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 5/6/22 3:11 PM, Claudio Fontana wrote:
> all the logic to open a fd, create a wrapper etc, is boilerplate
> code that is best reused, so change the Open function to take
> an existing already initialized virQEMUSaveFd.
> 
> Adapt all callers of qemuSaveImageOpen.
> 
> Signed-off-by: Claudio Fontana <cfontana@xxxxxxx>
> ---
>  src/qemu/qemu_driver.c    | 100 +++++++++++++++++++++++---------------
>  src/qemu/qemu_saveimage.c |  86 ++++++++------------------------
>  src/qemu/qemu_saveimage.h |   9 ++--
>  3 files changed, 82 insertions(+), 113 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index ff2be6ffe9..e4cdd93b50 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5825,12 +5825,13 @@ qemuDomainRestoreInternal(virConnectPtr conn,
>      virDomainObj *vm = NULL;
>      g_autofree char *xmlout = NULL;
>      const char *newxml = dxml;
> -    int fd = -1;
>      int ret = -1;
>      virQEMUSaveData *data = NULL;
> -    virFileWrapperFd *wrapperFd = NULL;
> +    virQEMUSaveFd saveFd = QEMU_SAVEFD_INVALID;
>      bool hook_taint = false;
>      bool reset_nvram = false;
> +    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
> +    int oflags = O_RDONLY;
>  
>      virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE |
>                    VIR_DOMAIN_SAVE_RUNNING |
> @@ -5841,10 +5842,18 @@ qemuDomainRestoreInternal(virConnectPtr conn,
>      if (flags & VIR_DOMAIN_SAVE_RESET_NVRAM)
>          reset_nvram = true;
>  
> -    fd = qemuSaveImageOpen(driver, NULL, path, &def, &data,
> -                           (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0,
> -                           &wrapperFd, false, false);
> -    if (fd < 0)
> +    if (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) {
> +        if (virFileDirectFdFlag() < 0) {
> +            virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> +                           _("bypass cache unsupported by this system"));
> +            return -1;
> +        }
> +        oflags |= O_DIRECT;
> +    }
> +    if (virQEMUSaveFdInit(&saveFd, path, 0, oflags, cfg) < 0)
> +        return -1;
> +
> +    if (qemuSaveImageOpen(driver, NULL, &def, &data, false, &saveFd) < 0)
>          goto cleanup;
>  
>      if (ensureACL(conn, def) < 0)
> @@ -5898,16 +5907,13 @@ qemuDomainRestoreInternal(virConnectPtr conn,
>                              flags) < 0)
>          goto cleanup;
>  
> -    ret = qemuSaveImageStartVM(conn, driver, vm, &fd, data, path,
> +    ret = qemuSaveImageStartVM(conn, driver, vm, &saveFd.fd, data, path,
>                                 false, reset_nvram, VIR_ASYNC_JOB_START);
>  
>      qemuProcessEndJob(vm);
>  
>   cleanup:
> -    VIR_FORCE_CLOSE(fd);
> -    if (virFileWrapperFdClose(wrapperFd) < 0)
> -        ret = -1;
> -    virFileWrapperFdFree(wrapperFd);
> +    ret = virQEMUSaveFdFini(&saveFd, vm, ret);
>      virQEMUSaveDataFree(data);
>      if (vm && ret < 0)
>          qemuDomainRemoveInactive(driver, vm);
> @@ -5969,15 +5975,16 @@ qemuDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *path,
>      virQEMUDriver *driver = conn->privateData;
>      char *ret = NULL;
>      g_autoptr(virDomainDef) def = NULL;
> -    int fd = -1;
>      virQEMUSaveData *data = NULL;
> +    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
> +    virQEMUSaveFd saveFd = QEMU_SAVEFD_INVALID;
>  
>      virCheckFlags(VIR_DOMAIN_SAVE_IMAGE_XML_SECURE, NULL);
>  
> -    fd = qemuSaveImageOpen(driver, NULL, path, &def, &data,
> -                           false, NULL, false, false);
> +    if (virQEMUSaveFdInit(&saveFd, path, 0, O_RDONLY, cfg) < 0)
> +        return NULL;
>  
> -    if (fd < 0)
> +    if (qemuSaveImageOpen(driver, NULL, &def, &data, false, &saveFd) < 0)
>          goto cleanup;
>  
>      if (virDomainSaveImageGetXMLDescEnsureACL(conn, def) < 0)
> @@ -5987,7 +5994,8 @@ qemuDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *path,
>  
>   cleanup:
>      virQEMUSaveDataFree(data);
> -    VIR_FORCE_CLOSE(fd);
> +    if (virQEMUSaveFdFini(&saveFd, NULL, ret ? 0 : -1) < 0)
> +        ret = NULL;
>      return ret;
>  }
>  
> @@ -5999,22 +6007,23 @@ qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path,
>      int ret = -1;
>      g_autoptr(virDomainDef) def = NULL;
>      g_autoptr(virDomainDef) newdef = NULL;
> -    int fd = -1;
>      virQEMUSaveData *data = NULL;
> +    virQEMUSaveFd saveFd = QEMU_SAVEFD_INVALID;
> +    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
>      int state = -1;
>  
>      virCheckFlags(VIR_DOMAIN_SAVE_RUNNING |
>                    VIR_DOMAIN_SAVE_PAUSED, -1);
>  
> +    if (virQEMUSaveFdInit(&saveFd, path, 0, O_RDWR, cfg) < 0)
> +        return -1;
> +
>      if (flags & VIR_DOMAIN_SAVE_RUNNING)
>          state = 1;
>      else if (flags & VIR_DOMAIN_SAVE_PAUSED)
>          state = 0;
>  
> -    fd = qemuSaveImageOpen(driver, NULL, path, &def, &data,
> -                           false, NULL, true, false);
> -
> -    if (fd < 0)
> +    if (qemuSaveImageOpen(driver, NULL, &def, &data, false, &saveFd) < 0)
>          goto cleanup;
>  
>      if (virDomainSaveImageDefineXMLEnsureACL(conn, def) < 0)
> @@ -6041,15 +6050,15 @@ qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path,
>                                               VIR_DOMAIN_XML_MIGRATABLE)))
>          goto cleanup;
>  
> -    if (lseek(fd, 0, SEEK_SET) != 0) {
> +    if (lseek(saveFd.fd, 0, SEEK_SET) != 0) {
>          virReportSystemError(errno, _("cannot seek in '%s'"), path);
>          goto cleanup;
>      }
>  
> -    if (virQEMUSaveDataWrite(data, fd, path) < 0)
> +    if (virQEMUSaveDataWrite(data, saveFd.fd, path) < 0)
>          goto cleanup;
>  
> -    if (VIR_CLOSE(fd) < 0) {
> +    if (virQEMUSaveFdClose(&saveFd, NULL) < 0) {
>          virReportSystemError(errno, _("failed to write header data to '%s'"), path);
>          goto cleanup;
>      }
> @@ -6057,8 +6066,8 @@ qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path,
>      ret = 0;
>  
>   cleanup:
> -    VIR_FORCE_CLOSE(fd);
>      virQEMUSaveDataFree(data);
> +    ret = virQEMUSaveFdFini(&saveFd, NULL, ret);
>      return ret;
>  }
>  
> @@ -6070,8 +6079,9 @@ qemuDomainManagedSaveGetXMLDesc(virDomainPtr dom, unsigned int flags)
>      g_autofree char *path = NULL;
>      char *ret = NULL;
>      g_autoptr(virDomainDef) def = NULL;
> -    int fd = -1;
>      virQEMUSaveData *data = NULL;
> +    virQEMUSaveFd saveFd = QEMU_SAVEFD_INVALID;
> +    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
>      qemuDomainObjPrivate *priv;
>  
>      virCheckFlags(VIR_DOMAIN_SAVE_IMAGE_XML_SECURE, NULL);
> @@ -6093,15 +6103,19 @@ qemuDomainManagedSaveGetXMLDesc(virDomainPtr dom, unsigned int flags)
>          goto cleanup;
>      }
>  
> -    if ((fd = qemuSaveImageOpen(driver, priv->qemuCaps, path, &def, &data,
> -                                false, NULL, false, false)) < 0)
> +    if (virQEMUSaveFdInit(&saveFd, path, 0, O_RDONLY, cfg) < 0)
> +        goto cleanup;
> +
> +    if (qemuSaveImageOpen(driver, priv->qemuCaps, &def, &data, false,
> +                          &saveFd) < 0)
>          goto cleanup;
>  
>      ret = qemuDomainDefFormatXML(driver, priv->qemuCaps, def, flags);
>  
>   cleanup:
>      virQEMUSaveDataFree(data);
> -    VIR_FORCE_CLOSE(fd);
> +    if (virQEMUSaveFdFini(&saveFd, vm, ret ? 0 : -1) < 0)
> +        ret = NULL;
>      virDomainObjEndAPI(&vm);
>      return ret;
>  }
> @@ -6152,16 +6166,25 @@ qemuDomainObjRestore(virConnectPtr conn,
>  {
>      g_autoptr(virDomainDef) def = NULL;
>      qemuDomainObjPrivate *priv = vm->privateData;
> -    int fd = -1;
>      int ret = -1;
>      g_autofree char *xmlout = NULL;
>      virQEMUSaveData *data = NULL;
> -    virFileWrapperFd *wrapperFd = NULL;
> +    virQEMUSaveFd saveFd = QEMU_SAVEFD_INVALID;
> +    int oflags = O_RDONLY;
> +    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
>  
> -    fd = qemuSaveImageOpen(driver, NULL, path, &def, &data,
> -                           bypass_cache, &wrapperFd, false, true);
> -    if (fd < 0) {
> -        if (fd == -3)
> +    if (bypass_cache) {
> +        if (virFileDirectFdFlag() < 0) {
> +            virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> +                           _("bypass cache unsupported by this system"));
> +            return -1;
> +        }
> +        oflags |= O_DIRECT;
> +    }
> +
> +    ret = qemuSaveImageOpen(driver, NULL, &def, &data, true, &saveFd);
> +    if (ret < 0) {
> +        if (ret == -3)
>              ret = 1;
>          goto cleanup;
>      }
> @@ -6205,15 +6228,12 @@ qemuDomainObjRestore(virConnectPtr conn,
>  
>      virDomainObjAssignDef(vm, &def, true, NULL);
>  
> -    ret = qemuSaveImageStartVM(conn, driver, vm, &fd, data, path,
> +    ret = qemuSaveImageStartVM(conn, driver, vm, &saveFd.fd, data, path,
>                                 start_paused, reset_nvram, asyncJob);
>  
>   cleanup:
>      virQEMUSaveDataFree(data);
> -    VIR_FORCE_CLOSE(fd);
> -    if (virFileWrapperFdClose(wrapperFd) < 0)
> -        ret = -1;
> -    virFileWrapperFdFree(wrapperFd);
> +    ret = virQEMUSaveFdFini(&saveFd, vm, ret);
>      return ret;
>  }
>  
> diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
> index 91fba7bd7d..5a569fa52e 100644
> --- a/src/qemu/qemu_saveimage.c
> +++ b/src/qemu/qemu_saveimage.c
> @@ -518,94 +518,49 @@ qemuSaveImageGetCompressionProgram(const char *imageFormat,
>   * @path: path of the save image
>   * @ret_def: returns domain definition created from the XML stored in the image
>   * @ret_data: returns structure filled with data from the image header
> - * @bypass_cache: bypass cache when opening the file
> - * @wrapperFd: returns the file wrapper structure
> - * @open_write: open the file for writing (for updates)
> - * @unlink_corrupt: remove the image file if it is corrupted
> + * @unlink_corrupt: mark the image file for removal if it is corrupted
> + * @saveFd: the save file
>   *
> - * Returns the opened fd of the save image file and fills the appropriate fields
> - * on success. On error returns -1 on most failures, -3 if corrupt image was
> - * unlinked (no error raised).
> + * Returns 0 on success or -1 on failure.
> + * On success, the appropriate fields are filled.
>   */
>  int
>  qemuSaveImageOpen(virQEMUDriver *driver,
>                    virQEMUCaps *qemuCaps,
> -                  const char *path,
>                    virDomainDef **ret_def,
>                    virQEMUSaveData **ret_data,
> -                  bool bypass_cache,
> -                  virFileWrapperFd **wrapperFd,
> -                  bool open_write,
> -                  bool unlink_corrupt)
> +                  bool unlink_corrupt,
> +                  virQEMUSaveFd *saveFd)
>  {
> -    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
> -    VIR_AUTOCLOSE fd = -1;
> -    int ret = -1;
>      g_autoptr(virQEMUSaveData) data = NULL;
>      virQEMUSaveHeader *header;
>      g_autoptr(virDomainDef) def = NULL;
> -    int oflags = open_write ? O_RDWR : O_RDONLY;
>      size_t xml_len;
>      size_t cookie_len;
>  
> -    if (bypass_cache) {
> -        int directFlag = virFileDirectFdFlag();
> -        if (directFlag < 0) {
> -            virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> -                           _("bypass cache unsupported by this system"));
> -            return -1;
> -        }
> -        oflags |= directFlag;
> -    }
> -
> -    if ((fd = qemuDomainOpenFile(cfg, NULL, path, oflags, NULL)) < 0)
> -        return -1;
> -
> -    if (bypass_cache &&
> -        !(*wrapperFd = virFileWrapperFdNew(&fd, path,
> -                                           VIR_FILE_WRAPPER_BYPASS_CACHE)))
> -        return -1;
>  
>      data = g_new0(virQEMUSaveData, 1);
>  
>      header = &data->header;
> -    if (saferead(fd, header, sizeof(*header)) != sizeof(*header)) {
> -        if (unlink_corrupt) {
> -            if (unlink(path) < 0) {
> -                virReportSystemError(errno,
> -                                     _("cannot remove corrupt file: %s"),
> -                                     path);
> -                return -1;
> -            } else {
> -                return -3;
> -            }
> -        }
> -
> +    if (saferead(saveFd->fd, header, sizeof(*header)) != sizeof(*header)) {
>          virReportError(VIR_ERR_OPERATION_FAILED,
>                         "%s", _("failed to read qemu header"));
> +        if (unlink_corrupt) {
> +            saveFd->need_unlink = true;
> +        }
>          return -1;
>      }
>  
>      if (memcmp(header->magic, QEMU_SAVE_MAGIC, sizeof(header->magic)) != 0) {
> -        if (memcmp(header->magic, QEMU_SAVE_PARTIAL, sizeof(header->magic)) == 0) {
> -            if (unlink_corrupt) {
> -                if (unlink(path) < 0) {
> -                    virReportSystemError(errno,
> -                                         _("cannot remove corrupt file: %s"),
> -                                         path);
> -                    return -1;
> -                } else {
> -                    return -3;
> -                }
> -            }
> -


Here the intentions of the original code are not 100% clear to me.

What do we want to do if header->magic does not match QEMU_SAVE_MAGIC?

- QEMU_SAVE_PARTIAL image : what do we want to do with that if unlink_corrupt is true, destroy it? This is what the code currently does.
- Other image (header does not match anything): what do we want to do with that if unlink_corrupt is true? Currently we return error in this case.


Is this really the original intention of the code?
Or was it rather the intention to keep QEMU_SAVE_PARTIAL images around, and delete the other?

I'll rewrite all of this in the next spin to match the original code if I don't hear anything, but I suspected that it might not be doing what was really intended.

Thanks,

Claudio



> +        if (memcmp(header->magic, QEMU_SAVE_PARTIAL, sizeof(header->magic)) == 0)
>              virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>                             _("save image is incomplete"));
> -            return -1;
> +        else
> +            virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> +                           _("image magic is incorrect"));
> +        if (unlink_corrupt) {
> +            saveFd->need_unlink = true;
>          }
> -
> -        virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> -                       _("image magic is incorrect"));
>          return -1;
>      }
>  
> @@ -636,7 +591,7 @@ qemuSaveImageOpen(virQEMUDriver *driver,
>  
>      data->xml = g_new0(char, xml_len);
>  
> -    if (saferead(fd, data->xml, xml_len) != xml_len) {
> +    if (saferead(saveFd->fd, data->xml, xml_len) != xml_len) {
>          virReportError(VIR_ERR_OPERATION_FAILED,
>                         "%s", _("failed to read domain XML"));
>          return -1;
> @@ -645,7 +600,7 @@ qemuSaveImageOpen(virQEMUDriver *driver,
>      if (cookie_len > 0) {
>          data->cookie = g_new0(char, cookie_len);
>  
> -        if (saferead(fd, data->cookie, cookie_len) != cookie_len) {
> +        if (saferead(saveFd->fd, data->cookie, cookie_len) != cookie_len) {
>              virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>                             _("failed to read cookie"));
>              return -1;
> @@ -661,10 +616,7 @@ qemuSaveImageOpen(virQEMUDriver *driver,
>      *ret_def = g_steal_pointer(&def);
>      *ret_data = g_steal_pointer(&data);
>  
> -    ret = fd;
> -    fd = -1;
> -
> -    return ret;
> +    return 0;
>  }
>  
>  int
> diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h
> index 41937e5eb5..5dc63f3661 100644
> --- a/src/qemu/qemu_saveimage.h
> +++ b/src/qemu/qemu_saveimage.h
> @@ -92,14 +92,11 @@ qemuSaveImageStartVM(virConnectPtr conn,
>  int
>  qemuSaveImageOpen(virQEMUDriver *driver,
>                    virQEMUCaps *qemuCaps,
> -                  const char *path,
>                    virDomainDef **ret_def,
>                    virQEMUSaveData **ret_data,
> -                  bool bypass_cache,
> -                  virFileWrapperFd **wrapperFd,
> -                  bool open_write,
> -                  bool unlink_corrupt)
> -    ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4);
> +                  bool unlink_corrupt,
> +                  virQEMUSaveFd *saveFd)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(6);
>  
>  int
>  qemuSaveImageGetCompressionProgram(const char *imageFormat,
> 




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux