On 09/30/2014 08:20 PM, Tomoki Sekiyama wrote:
> Add daemon and driver code to (de-)serialize virDomainFSInfo.
>
> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@xxxxxxx>
> ---
> daemon/remote.c | 117 ++++++++++++++++++++++++++++++++++++++++++
> src/remote/remote_driver.c | 87 +++++++++++++++++++++++++++++++
> src/remote/remote_protocol.x | 32 +++++++++++
> src/remote_protocol-structs | 21 ++++++++
> src/rpc/gendispatch.pl | 1
> 5 files changed, 257 insertions(+), 1 deletion(-)
>
> diff --git a/daemon/remote.c b/daemon/remote.c
> index c93c97a..d775434 100644
> --- a/daemon/remote.c
> +++ b/daemon/remote.c
> @@ -6340,6 +6340,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;
> +
> + 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) {
Because you compare 'info' to NULL here, Coverity gets grumpy later on [1]
> + 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;
Because we won't get here if virDomainGetFSInfo() fails, then the loop
below [2] will cause a problem...
The good news is if you move this to above the virDomainGetFSInfo()
call, then you avoid the Coverity complaint.
> + }
> +
> + ret->ret = ninfo;
> +
> + rv = 0;
> +
> + cleanup:
> + if (rv < 0) {
> + virNetMessageSaveError(rerr);
> +
> + if (ret->info.info_val) {
> + for (i = 0; i < ninfo; i++) {
[2]
Coverity complains when virDomainGetFSInfo() fails leaving ninfo == -1
> + 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)
You'll need the { at the end (clarity) and because of [1], make it
if (ninfo >= 0 && info) {
> + for (i = 0; i < ninfo; i++)
[1]
And this is where Coverity gets grumpy indicating 'info' could be NULL
and this would be a NULL deref. Yes, a false positive when you know the
code, but nonetheless
> + virDomainFSInfoFree(info[i]);
and the corresponding }
> + VIR_FREE(info);
> +
> + return rv;
> +}
> +
> +
> /*----- Helpers. -----*/
>
> /* get_nonnull_domain and get_nonnull_network turn an on-wire
> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index 6c49e49..aa1866a 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -7897,6 +7897,92 @@ 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->networkPrivateData;
> + 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 &&
> + VIR_ALLOC_N(info_ret, ret.info.info_len + 1) < 0)
I see no reason for the + 1 since you're returning a count.
> + 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,
> @@ -8239,6 +8325,7 @@ static virDriver remote_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 db12cda..bf261e1 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. */
> @@ -5505,5 +5529,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..e002b2a 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;
> + } leases;
> + u_int ret;
> +};
make check fails...
s/leases/info
resolves it.
John
> 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 27093d2..d2cfffb 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/;
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list