Re: [PATCH v3 03/11] admin: Introduce virAdmConnectIsAlive

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

On Fri, Nov 06, 2015 at 12:46:18PM +0100, Erik Skultety wrote:
> Since most of our APIs rely on an acive functional connection to a daemon and
> we have such a mechanism in libvirt already, there's need to have such a way in
> libvirt-admin as well. By introducing a new public API, this patch provides
> support to check for an active connection.
> ---
> include/libvirt/libvirt-admin.h |  1 +
> src/libvirt-admin.c             | 29 +++++++++++++++++++++++++++++
> src/libvirt_admin_public.syms   |  1 +
> tools/virt-admin.c              |  4 ++--
> 4 files changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h
> index 9997cc2..72671c6 100644
> --- a/include/libvirt/libvirt-admin.h
> +++ b/include/libvirt/libvirt-admin.h
> @@ -52,6 +52,7 @@ virAdmConnectPtr virAdmConnectOpen(const char *name, unsigned int flags);
> int virAdmConnectClose(virAdmConnectPtr conn);
>
> int virAdmConnectRef(virAdmConnectPtr conn);
> +int virAdmConnectIsAlive(virAdmConnectPtr conn);
>
> # ifdef __cplusplus
> }
> diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c
> index 5a4fc48..105727f 100644
> --- a/src/libvirt-admin.c
> +++ b/src/libvirt-admin.c
> @@ -386,3 +386,32 @@ virAdmConnectRef(virAdmConnectPtr conn)
>
>     return 0;
> }
> +
> +/**
> + * virAdmConnectIsAlive:
> + * @conn: connection to admin server
> + *
> + * Decide whether the connection to the admin server is alive or not.
> + * Connection is considered alive if the channel it is running over is not
> + * closed.
> + *
> + * Returns 1, if the connection is alive, 0 if the channel has already
> + * been closed or -1 on error.
> + */
> +int
> +virAdmConnectIsAlive(virAdmConnectPtr conn)
> +{
> +    bool ret;
> +    remoteAdminPrivPtr priv = conn->privateData;
> +

I know it wouldn't be consistent with virConnectIsAlive(), but we
could handle NULLs nicely, e.g. just return 0.  I guess -1 is OK as
well because everyone will check for '== 1' anyway (if at all ever).

> +    VIR_DEBUG("conn=%p", conn);
> +
> +    virResetLastError();
> +    virCheckAdmConnectReturn(conn, -1);
> +
> +    virObjectLock(priv);
> +    ret = virNetClientIsOpen(priv->client);
> +    virObjectUnlock(priv);
> +
> +    return ret;
> +}
> diff --git a/src/libvirt_admin_public.syms b/src/libvirt_admin_public.syms
> index d9e3c0b..16cfd42 100644
> --- a/src/libvirt_admin_public.syms
> +++ b/src/libvirt_admin_public.syms
> @@ -15,4 +15,5 @@ LIBVIRT_ADMIN_1.3.0 {
>         virAdmConnectOpen;
>         virAdmConnectClose;
>         virAdmConnectRef;
> +        virAdmConnectIsAlive;
> };
> diff --git a/tools/virt-admin.c b/tools/virt-admin.c
> index ddfba91..6b009ea 100644
> --- a/tools/virt-admin.c
> +++ b/tools/virt-admin.c
> @@ -159,10 +159,10 @@ vshAdmConnectionHandler(vshControl *ctl)
> {
>     vshAdmControlPtr priv = ctl->privData;
>
> -    if (!priv->conn)
> +    if (!priv->conn || !virAdmConnectIsAlive(priv->conn))
>         vshAdmReconnect(ctl);

you could then drop the '!priv->conn' here...

ACK either way if nobody else has an opinion on that for some time.
Just remember that, you need to adjust the documentation for
virAdmConnectIsAlive() if you change its behaviour.

> -    if (!priv->conn) {
> +    if (!priv->conn || !virAdmConnectIsAlive(priv->conn)) {
>         vshError(ctl, "%s", _("no valid connection"));
>         return NULL;
>     }
> --
> 2.4.3
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: signature.asc
Description: PGP signature

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