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

Re: [libvirt] [PATCH v2 2/5] remote: Implement the remote protocol for virDomainGetFSInfo




On 11/17/2014 06:26 PM, Tomoki Sekiyama wrote:
> Add daemon and driver code to (de-)serialize virDomainFSInfo.
> 
> Signed-off-by: Tomoki Sekiyama <tomoki sekiyama hds com>
> ---
>  daemon/remote.c              |  117 ++++++++++++++++++++++++++++++++++++++++++
>  src/remote/remote_driver.c   |   92 +++++++++++++++++++++++++++++++++
>  src/remote/remote_protocol.x |   32 +++++++++++
>  src/remote_protocol-structs  |   21 ++++++++
>  src/rpc/gendispatch.pl       |    1 
>  5 files changed, 262 insertions(+), 1 deletion(-)
> 
> diff --git a/daemon/remote.c b/daemon/remote.c
> index 1d7082e..9b89fd0 100644
> --- a/daemon/remote.c
> +++ b/daemon/remote.c
> @@ -6336,6 +6336,123 @@ remoteDispatchNodeAllocPages(virNetServerPtr server ATTRIBUTE_UNUSED,
>  }
>  
>  
> +static int
> +remoteDispatchDomainGetFSInfo(virNetServerPtr server ATTRIBUTE_UNUSED,
> +                              virNetServerClientPtr client,
> +                              virNetMessagePtr msg ATTRIBUTE_UNUSED,
> +                              virNetMessageErrorPtr rerr,
> +                              remote_domain_get_fsinfo_args *args,
> +                              remote_domain_get_fsinfo_ret *ret)
> +{
> +    int rv = -1;
> +    size_t i, j;
> +    struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client);
> +    virDomainFSInfoPtr *info = NULL;
> +    virDomainPtr dom = NULL;
> +    remote_domain_fsinfo *dst;
> +    char **alias;
> +    int ninfo = 0, ndisk;
> +
> +    if (!priv->conn) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
> +        goto cleanup;
> +    }
> +
> +    if (!(dom = get_nonnull_domain(priv->conn, args->dom)))
> +        goto cleanup;
> +
> +    if ((ninfo = virDomainGetFSInfo(dom, &info, args->flags)) < 0)
> +        goto cleanup;

[2] Since ninfo can be negative here... rv == -1 and ret->info.info_val
is whatever it was passed in...

> +
> +    if (ninfo > REMOTE_DOMAIN_FSINFO_MAX) {
> +        virReportError(VIR_ERR_RPC,
> +                       _("Too many mountpoints in fsinfo: %d for limit %d"),
> +                       ninfo, REMOTE_DOMAIN_FSINFO_MAX);
> +        goto cleanup;
> +    }
> +
> +    if (info && ninfo) {

Here again, check both info & ninfo here causes Coverity to complain
later [1].

If we just check 'ninfo' here, then the else condition takes care of
setting the ret->info.info_val and ret->info.info_len.

Thus this if (ninfo) can handle things when 'info' has something returned.

> +        if (VIR_ALLOC_N(ret->info.info_val, ninfo) < 0)
> +            goto cleanup;
> +
> +        ret->info.info_len = ninfo;
> +
> +        for (i = 0; i < ninfo; i++) {
> +            dst = &ret->info.info_val[i];
> +            if (VIR_STRDUP(dst->mountpoint, info[i]->mountpoint) < 0)
> +                goto cleanup;
> +
> +            if (VIR_STRDUP(dst->name, info[i]->name) < 0)
> +                goto cleanup;
> +
> +            if (VIR_STRDUP(dst->type, info[i]->type) < 0)
> +                goto cleanup;
> +
> +            ndisk = 0;
> +            for (alias = info[i]->devAlias; alias && *alias; alias++)
> +                ndisk++;
> +
> +            if (ndisk > REMOTE_DOMAIN_FSINFO_DISKS_MAX) {
> +                virReportError(VIR_ERR_RPC,
> +                               _("Too many disks in fsinfo: %d for limit %d"),
> +                               ndisk, REMOTE_DOMAIN_FSINFO_DISKS_MAX);
> +                goto cleanup;
> +            }
> +
> +            if (ndisk > 0) {
> +                if (VIR_ALLOC_N(dst->dev_aliases.dev_aliases_val, ndisk) < 0)
> +                    goto cleanup;
> +
> +                for (j = 0; j < ndisk; j++) {
> +                    if (VIR_STRDUP(dst->dev_aliases.dev_aliases_val[j],
> +                                   info[i]->devAlias[j]) < 0)
> +                        goto cleanup;
> +                }
> +
> +                dst->dev_aliases.dev_aliases_len = ndisk;
> +            } else {
> +                dst->dev_aliases.dev_aliases_val = NULL;
> +                dst->dev_aliases.dev_aliases_len = 0;
> +            }
> +        }
> +
> +    } else {
> +        ret->info.info_len = 0;
> +        ret->info.info_val = NULL;
> +    }
> +
> +    ret->ret = ninfo;
> +
> +    rv = 0;
> +
> + cleanup:
> +    if (rv < 0) {
> +        virNetMessageSaveError(rerr);
> +
> +        if (ret->info.info_val) {
> +            for (i = 0; i < ninfo; i++) {

[2] Coverity has issues here because we can get here with ninfo < 0.
Since it seems we really only care to enter this loop when
ret->info.info_val && ninfo > 0, we can do that

> +                dst = &ret->info.info_val[i];
> +                VIR_FREE(dst->mountpoint);
> +                if (dst->dev_aliases.dev_aliases_val) {
> +                    for (j = 0; j < dst->dev_aliases.dev_aliases_len; j++)
> +                        VIR_FREE(dst->dev_aliases.dev_aliases_val[j]);
> +                    VIR_FREE(dst->dev_aliases.dev_aliases_val);
> +                }
> +            }
> +            VIR_FREE(ret->info.info_val);
> +        }
> +    }
> +    if (dom)
> +        virDomainFree(dom);
> +    if (ninfo >= 0)
> +        for (i = 0; i < ninfo; i++)
> +            virDomainFSInfoFree(info[i]);

[1] Because you checked 'ninfo' and 'info' above, Coverity complains
here because you're not checking if 'info' (yes, even though we know
it's obvious
> +    VIR_FREE(info);
> +
> +    return rv;
> +}
> +
> +
>  /*----- Helpers. -----*/
>  
>  /* get_nonnull_domain and get_nonnull_network turn an on-wire

I will squash the following in:

diff --git a/daemon/remote.c b/daemon/remote.c
index 9b89fd0..4439af7 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -6371,7 +6371,7 @@ remoteDispatchDomainGetFSInfo(virNetServerPtr
server ATTRI
         goto cleanup;
     }

-    if (info && ninfo) {
+    if (ninfo) {
         if (VIR_ALLOC_N(ret->info.info_val, ninfo) < 0)
             goto cleanup;

@@ -6429,7 +6429,7 @@ remoteDispatchDomainGetFSInfo(virNetServerPtr
server ATTRI
     if (rv < 0) {
         virNetMessageSaveError(rerr);

-        if (ret->info.info_val) {
+        if (ret->info.info_val && ninfo > 0) {
             for (i = 0; i < ninfo; i++) {
                 dst = &ret->info.info_val[i];
                 VIR_FREE(dst->mountpoint);

> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index 04e5360..4c0758f 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -7829,6 +7829,97 @@ remoteNodeAllocPages(virConnectPtr conn,
>  }
>  
>  
> +static int
> +remoteDomainGetFSInfo(virDomainPtr dom,
> +                      virDomainFSInfoPtr **info,
> +                      unsigned int flags)
> +{
> +    int rv = -1;
> +    size_t i, j, len;
> +    struct private_data *priv = dom->conn->privateData;
> +    remote_domain_get_fsinfo_args args;
> +    remote_domain_get_fsinfo_ret ret;
> +    remote_domain_fsinfo *src;
> +    virDomainFSInfoPtr *info_ret = NULL;
> +
> +    remoteDriverLock(priv);
> +
> +    make_nonnull_domain(&args.dom, dom);
> +
> +    args.flags = flags;
> +
> +    memset(&ret, 0, sizeof(ret));
> +
> +    if (call(dom->conn, priv, 0, REMOTE_PROC_DOMAIN_GET_FSINFO,
> +             (xdrproc_t)xdr_remote_domain_get_fsinfo_args, (char *)&args,
> +             (xdrproc_t)xdr_remote_domain_get_fsinfo_ret, (char *)&ret) == -1)
> +        goto done;
> +
> +    if (ret.info.info_len > REMOTE_DOMAIN_FSINFO_MAX) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Too many mountpoints in fsinfo: %d for limit %d"),
> +                       ret.info.info_len, REMOTE_DOMAIN_FSINFO_MAX);
> +        goto cleanup;
> +    }
> +
> +    if (info) {
> +        if (!ret.info.info_len) {
> +            *info = NULL;
> +            rv = ret.ret;
> +            goto cleanup;
> +        }
> +
> +        if (VIR_ALLOC_N(info_ret, ret.info.info_len) < 0)
> +            goto cleanup;
> +
> +        for (i = 0; i < ret.info.info_len; i++) {
> +            src = &ret.info.info_val[i];
> +
> +            if (VIR_ALLOC(info_ret[i]) < 0)
> +                goto cleanup;
> +
> +            if (VIR_STRDUP(info_ret[i]->mountpoint, src->mountpoint) < 0)
> +                goto cleanup;
> +
> +            if (VIR_STRDUP(info_ret[i]->name, src->name) < 0)
> +                goto cleanup;
> +
> +            if (VIR_STRDUP(info_ret[i]->type, src->type) < 0)
> +                goto cleanup;
> +
> +            len = src->dev_aliases.dev_aliases_len;
> +            if (len &&
> +                VIR_ALLOC_N(info_ret[i]->devAlias, len + 1) < 0)
> +                goto cleanup;
> +
> +            for (j = 0; j < len; j++) {
> +                if (VIR_STRDUP(info_ret[i]->devAlias[j],
> +                               src->dev_aliases.dev_aliases_val[j]) < 0)
> +                    goto cleanup;
> +            }
> +        }
> +
> +        *info = info_ret;
> +        info_ret = NULL;
> +    }
> +
> +    rv = ret.ret;
> +
> + cleanup:
> +    if (info_ret) {
> +        for (i = 0; i < ret.info.info_len; i++)
> +            virDomainFSInfoFree(info_ret[i]);
> +        VIR_FREE(info_ret);
> +    }
> +    xdr_free((xdrproc_t)xdr_remote_domain_get_fsinfo_ret,
> +             (char *) &ret);
> +
> + done:
> +    remoteDriverUnlock(priv);
> +    return rv;
> +}
> +
> +
>  /* get_nonnull_domain and get_nonnull_network turn an on-wire
>   * (name, uuid) pair into virDomainPtr or virNetworkPtr object.
>   * These can return NULL if underlying memory allocations fail,
> @@ -8171,6 +8262,7 @@ static virHypervisorDriver hypervisor_driver = {
>      .connectGetDomainCapabilities = remoteConnectGetDomainCapabilities, /* 1.2.7 */
>      .connectGetAllDomainStats = remoteConnectGetAllDomainStats, /* 1.2.8 */
>      .nodeAllocPages = remoteNodeAllocPages, /* 1.2.9 */
> +    .domainGetFSInfo = remoteDomainGetFSInfo, /* 1.2.10 */
>  };
>  
>  static virNetworkDriver network_driver = {
> diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
> index ebf4530..10c8068 100644
> --- a/src/remote/remote_protocol.x
> +++ b/src/remote/remote_protocol.x
> @@ -250,6 +250,12 @@ const REMOTE_CONNECT_GET_ALL_DOMAIN_STATS_MAX = 4096;
>  /* Upper limit of message size for tunable event. */
>  const REMOTE_DOMAIN_EVENT_TUNABLE_MAX = 2048;
>  
> +/* Upper limit on number of mountpoints in fsinfo */
> +const REMOTE_DOMAIN_FSINFO_MAX = 256;
> +
> +/* Upper limit on number of disks per mountpoint in fsinfo */
> +const REMOTE_DOMAIN_FSINFO_DISKS_MAX = 256;
> +
>  /* UUID.  VIR_UUID_BUFLEN definition comes from libvirt.h */
>  typedef opaque remote_uuid[VIR_UUID_BUFLEN];
>  
> @@ -3111,6 +3117,24 @@ struct remote_connect_get_all_domain_stats_args {
>  struct remote_connect_get_all_domain_stats_ret {
>      remote_domain_stats_record retStats<REMOTE_DOMAIN_LIST_MAX>;
>  };
> +
> +struct remote_domain_fsinfo {
> +    remote_nonnull_string mountpoint;
> +    remote_nonnull_string name;
> +    remote_nonnull_string type;
> +    remote_nonnull_string dev_aliases<REMOTE_DOMAIN_FSINFO_DISKS_MAX>; /* (const char **) */
> +};
> +
> +struct remote_domain_get_fsinfo_args {
> +    remote_nonnull_domain dom;
> +    unsigned int flags;
> +};
> +
> +struct remote_domain_get_fsinfo_ret {
> +    remote_domain_fsinfo info<REMOTE_DOMAIN_FSINFO_MAX>;
> +    unsigned int ret;
> +};
> +
>  /*----- Protocol. -----*/
>  
>  /* Define the program number, protocol version and procedure numbers here. */
> @@ -5506,5 +5530,11 @@ enum remote_procedure {
>       * @generate: none
>       * @acl: connect:write
>       */
> -    REMOTE_PROC_NODE_ALLOC_PAGES = 347
> +    REMOTE_PROC_NODE_ALLOC_PAGES = 347,
> +
> +    /**
> +     * @generate: none
> +     * @acl: domain:read
> +     */
> +    REMOTE_PROC_DOMAIN_GET_FSINFO = 348
>  };
> diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
> index 362baf9..6419102 100644
> --- a/src/remote_protocol-structs
> +++ b/src/remote_protocol-structs
> @@ -2579,6 +2579,26 @@ struct remote_connect_get_all_domain_stats_ret {
>                  remote_domain_stats_record * retStats_val;
>          } retStats;
>  };
> +struct remote_domain_fsinfo {
> +        remote_nonnull_string      mountpoint;
> +        remote_nonnull_string      name;
> +        remote_nonnull_string      type;
> +        struct {
> +                u_int              dev_aliases_len;
> +                remote_nonnull_string * dev_aliases_val;
> +        } dev_aliases;
> +};
> +struct remote_domain_get_fsinfo_args {
> +        remote_nonnull_domain      dom;
> +        u_int                      flags;
> +};
> +struct remote_domain_get_fsinfo_ret {
> +        struct {
> +                u_int              info_len;
> +                remote_domain_fsinfo * info_val;
> +        } info;
> +        u_int                      ret;
> +};
>  enum remote_procedure {
>          REMOTE_PROC_CONNECT_OPEN = 1,
>          REMOTE_PROC_CONNECT_CLOSE = 2,
> @@ -2927,4 +2947,5 @@ enum remote_procedure {
>          REMOTE_PROC_DOMAIN_BLOCK_COPY = 345,
>          REMOTE_PROC_DOMAIN_EVENT_CALLBACK_TUNABLE = 346,
>          REMOTE_PROC_NODE_ALLOC_PAGES = 347,
> +        REMOTE_PROC_DOMAIN_GET_FSINFO = 348,
>  };
> diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl
> index b38d5bb..80f35b3 100755
> --- a/src/rpc/gendispatch.pl
> +++ b/src/rpc/gendispatch.pl
> @@ -66,6 +66,7 @@ sub fixup_name {
>      $name =~ s/Fstrim$/FSTrim/;
>      $name =~ s/Fsfreeze$/FSFreeze/;
>      $name =~ s/Fsthaw$/FSThaw/;
> +    $name =~ s/Fsinfo$/FSInfo/;
>      $name =~ s/Scsi/SCSI/;
>      $name =~ s/Wwn$/WWN/;
>      $name =~ s/Dhcp$/DHCP/;
> 


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