[libvirt] [PATCH] remote: check & report OOM in make_nonnull_XXX methods

[Michal Privoznik]

On 12/11/18 4:56 PM, Daniel P. Berrangé wrote:
> The make_nonnull_XXX methods can all fail due to OOM but this was being
> silently ignored and thus also not checked by callers. Make the methods
> propagate errors and use ATTRIBUTE_RETURN_CHECK to force callers to deal
> with it.
> 
> Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> ---
>  src/admin/admin_server_dispatch.c   |   9 +-
>  src/remote/remote_daemon_dispatch.c | 393 +++++++++++++++++++++-------
>  src/rpc/gendispatch.pl              |  19 +-
>  3 files changed, 315 insertions(+), 106 deletions(-)
> 
> diff --git a/src/admin/admin_server_dispatch.c b/src/admin/admin_server_dispatch.c
> index b78ff902c0..9fa2893fa3 100644
> --- a/src/admin/admin_server_dispatch.c
> +++ b/src/admin/admin_server_dispatch.c
> @@ -115,11 +115,13 @@ get_nonnull_server(virNetDaemonPtr dmn, admin_nonnull_server srv)
>      return virNetDaemonGetServer(dmn, srv.name);
>  }
>  
> -static void
> +static int ATTRIBUTE_RETURN_CHECK
>  make_nonnull_server(admin_nonnull_server *srv_dst,
>                      virNetServerPtr srv_src)
>  {
> -    ignore_value(VIR_STRDUP_QUIET(srv_dst->name, virNetServerGetName(srv_src)));
> +    if (VIR_STRDUP(srv_dst->name, virNetServerGetName(srv_src)) < 0)
> +        return -1;
> +    return 0;
>  }
>  
>  static virNetServerClientPtr
> @@ -128,13 +130,14 @@ get_nonnull_client(virNetServerPtr srv, admin_nonnull_client clnt)
>      return virNetServerGetClient(srv, clnt.id);
>  }
>  
> -static void
> +static int
>  make_nonnull_client(admin_nonnull_client *clt_dst,
>                      virNetServerClientPtr clt_src)
>  {
>      clt_dst->id = virNetServerClientGetID(clt_src);
>      clt_dst->timestamp = virNetServerClientGetTimestamp(clt_src);
>      clt_dst->transport = virNetServerClientGetTransport(clt_src);
> +    return 0;
>  }
>  
>  /* Functions */
> diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
> index e62ebfb596..73a9434700 100644
> --- a/src/remote/remote_daemon_dispatch.c
> +++ b/src/remote/remote_daemon_dispatch.c
> @@ -93,16 +93,16 @@ static virNWFilterPtr get_nonnull_nwfilter(virConnectPtr conn, remote_nonnull_nw
>  static virNWFilterBindingPtr get_nonnull_nwfilter_binding(virConnectPtr conn, remote_nonnull_nwfilter_binding binding);
>  static virDomainSnapshotPtr get_nonnull_domain_snapshot(virDomainPtr dom, remote_nonnull_domain_snapshot snapshot);
>  static virNodeDevicePtr get_nonnull_node_device(virConnectPtr conn, remote_nonnull_node_device dev);
> -static void make_nonnull_domain(remote_nonnull_domain *dom_dst, virDomainPtr dom_src);
> -static void make_nonnull_network(remote_nonnull_network *net_dst, virNetworkPtr net_src);
> -static void make_nonnull_interface(remote_nonnull_interface *interface_dst, virInterfacePtr interface_src);
> -static void make_nonnull_storage_pool(remote_nonnull_storage_pool *pool_dst, virStoragePoolPtr pool_src);
> -static void make_nonnull_storage_vol(remote_nonnull_storage_vol *vol_dst, virStorageVolPtr vol_src);
> -static void make_nonnull_node_device(remote_nonnull_node_device *dev_dst, virNodeDevicePtr dev_src);
> -static void make_nonnull_secret(remote_nonnull_secret *secret_dst, virSecretPtr secret_src);
> -static void make_nonnull_nwfilter(remote_nonnull_nwfilter *net_dst, virNWFilterPtr nwfilter_src);
> -static void make_nonnull_nwfilter_binding(remote_nonnull_nwfilter_binding *binding_dst, virNWFilterBindingPtr binding_src);
> -static void make_nonnull_domain_snapshot(remote_nonnull_domain_snapshot *snapshot_dst, virDomainSnapshotPtr snapshot_src);
> +static int make_nonnull_domain(remote_nonnull_domain *dom_dst, virDomainPtr dom_src) ATTRIBUTE_RETURN_CHECK;
> +static int make_nonnull_network(remote_nonnull_network *net_dst, virNetworkPtr net_src) ATTRIBUTE_RETURN_CHECK;
> +static int make_nonnull_interface(remote_nonnull_interface *interface_dst, virInterfacePtr interface_src) ATTRIBUTE_RETURN_CHECK;
> +static int make_nonnull_storage_pool(remote_nonnull_storage_pool *pool_dst, virStoragePoolPtr pool_src) ATTRIBUTE_RETURN_CHECK;
> +static int make_nonnull_storage_vol(remote_nonnull_storage_vol *vol_dst, virStorageVolPtr vol_src) ATTRIBUTE_RETURN_CHECK;
> +static int make_nonnull_node_device(remote_nonnull_node_device *dev_dst, virNodeDevicePtr dev_src) ATTRIBUTE_RETURN_CHECK;
> +static int make_nonnull_secret(remote_nonnull_secret *secret_dst, virSecretPtr secret_src) ATTRIBUTE_RETURN_CHECK;
> +static int make_nonnull_nwfilter(remote_nonnull_nwfilter *net_dst, virNWFilterPtr nwfilter_src) ATTRIBUTE_RETURN_CHECK;
> +static int make_nonnull_nwfilter_binding(remote_nonnull_nwfilter_binding *binding_dst, virNWFilterBindingPtr binding_src) ATTRIBUTE_RETURN_CHECK;
> +static int make_nonnull_domain_snapshot(remote_nonnull_domain_snapshot *snapshot_dst, virDomainSnapshotPtr snapshot_src) ATTRIBUTE_RETURN_CHECK;
>  
>  static int
>  remoteSerializeDomainDiskErrors(virDomainDiskErrorPtr errors,
> @@ -315,7 +315,8 @@ remoteRelayDomainEventLifecycle(virConnectPtr conn,
>  
>      /* build return data */
>      memset(&data, 0, sizeof(data));
> -    make_nonnull_domain(&data.dom, dom);
> +    if (make_nonnull_domain(&data.dom, dom) < 0)
> +        goto error;
>      data.event = event;
>      data.detail = detail;
>  
> @@ -335,6 +336,11 @@ remoteRelayDomainEventLifecycle(virConnectPtr conn,
>      }
>  
>      return 0;
> +
> + error:
> +    xdr_free((xdrproc_t)xdr_remote_domain_event_lifecycle_msg,
> +             &data);
> +    return -1;

Strictly speaking, this is not needed. But I see the rationale behind.

ACK

Michal