Re: [PATCH 16/16] backup: Introduce virDomainCheckpointPtr

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

 




On 3/20/19 1:41 AM, Eric Blake wrote:
> Prepare for introducing a bunch of new public APIs related to backup
> checkpoints by first introducing a new internal type and errors
> associated with that type.  Checkpoints are modeled heavily after
> virDomainSnapshotPtr (both represent a point in time of the guest),
> although a snapshot exists with the intent of rolling back to that
> state, while a checkpoint exists to make it possible to create an
> incremental backup at a later time.  Thus, it shares the common
> virDomainMoment base class created in the previous patches.
> 
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
>  include/libvirt/virterror.h |  6 +++++-
>  src/util/virerror.c         | 12 ++++++++++-
>  include/libvirt/libvirt.h   |  6 +++++-
>  src/datatypes.h             | 43 +++++++++++++++++++++++++++++++++++++
>  src/datatypes.c             | 22 +++++++++++++++++++
>  src/libvirt_private.syms    |  2 ++
>  6 files changed, 88 insertions(+), 3 deletions(-)
> 

You noted in a response a need to rework this due to removing patch 2&3,
which well probably isn't entirely necessary, but if you do, then it's
fine.  I've seen this in previous reviews as well and it generally looks
fine to me.

Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>

John

[one nit below]

> diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
> index 3c19ff5e2e..bccf3c731e 100644
> --- a/include/libvirt/virterror.h
> +++ b/include/libvirt/virterror.h
> @@ -4,7 +4,7 @@
>   * Description: Provides the interfaces of the libvirt library to handle
>   *              errors raised while using the library.
>   *
> - * Copyright (C) 2006-2016 Red Hat, Inc.
> + * Copyright (C) 2006-2019 Red Hat, Inc.
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public
> @@ -132,6 +132,7 @@ typedef enum {
>      VIR_FROM_LIBSSH = 66,       /* Error from libssh connection transport */
>      VIR_FROM_RESCTRL = 67,      /* Error from resource control */
>      VIR_FROM_FIREWALLD = 68,    /* Error from firewalld */
> +    VIR_FROM_DOMAIN_CHECKPOINT = 69,/* Error from domain checkpoint */

You could add a space between ,/ like you did below even though the
columnar formatting changed.

> 
>  # ifdef VIR_ENUM_SENTINELS
>      VIR_ERR_DOMAIN_LAST
> @@ -322,6 +323,9 @@ typedef enum {
>      VIR_ERR_DEVICE_MISSING = 99,        /* fail to find the desired device */
>      VIR_ERR_INVALID_NWFILTER_BINDING = 100,  /* invalid nwfilter binding */
>      VIR_ERR_NO_NWFILTER_BINDING = 101,  /* no nwfilter binding */
> +    VIR_ERR_INVALID_DOMAIN_CHECKPOINT = 102, /* invalid domain checkpoint */
> +    VIR_ERR_NO_DOMAIN_CHECKPOINT = 103, /* domain checkpoint not found */
> +    VIR_ERR_NO_DOMAIN_BACKUP = 104,     /* domain backup job id not found */
> 
>  # ifdef VIR_ENUM_SENTINELS
>      VIR_ERR_NUMBER_LAST

[...]

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[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