Re: [Xen-devel] [PATCH v3 08/15] argo: implement the unregister op

[Roger Pau Monné <royger@xxxxxxxxxxx>]

On Mon, Jan 7, 2019 at 8:44 AM Christopher Clark
<christopher.w.clark@xxxxxxxxx> wrote:
>
> Takes a single argument: a handle to the ring unregistration struct,
> which specifies the port and partner domain id or wildcard.
>
> The ring's entry is removed from the hashtable of registered rings;
> any entries for pending notifications are removed; and the ring is
> unmapped from Xen's address space.
>
> If the ring had been registered to communicate with a single specified
> domain (ie. a non-wildcard ring) then the partner domain state is removed
> from the partner domain's argo send_info hash table.
>
> Signed-off-by: Christopher Clark <christopher.clark6@xxxxxxxxxxxxxx>
> ---
> v2 feedback Jan: drop cookie, implement teardown
> v2 feedback Jan: drop message from argo_message_op
> v2 self: OVERHAUL
> v2 self: reorder logic to shorten critical section
> v1 #13 feedback Jan: revise use of guest_handle_okay vs __copy ops
> v1 feedback Roger, Jan: drop argo prefix on static functions
> v1,2 feedback Jan/Roger/Paul: drop errno returning guest access functions
> v1 #5 (#14) feedback Paul: use currd in do_argo_message_op
> v1 #5 (#14) feedback Paul: full use currd in argo_unregister_ring
> v1 #13 (#14) feedback Paul: replace do/while with goto; reindent
> v1 self: add blank lines in unregister case in do_argo_message_op
> v1: #13 feedback Jan: public namespace: prefix with xen
> v1: #13 feedback Jan: blank line after op case in do_argo_message_op
> v1: #14 feedback Jan: replace domain id override with validation
> v1: #18 feedback Jan: meld the ring count limit into the series
> v1: feedback #15 Jan: verify zero in unused hypercall args
>
>  xen/common/argo.c         | 115 
> ++++++++++++++++++++++++++++++++++++++++++++++
>  xen/include/public/argo.h |  19 ++++++++
>  xen/include/xlat.lst      |   1 +
>  3 files changed, 135 insertions(+)
>
> diff --git a/xen/common/argo.c b/xen/common/argo.c
> index 11988e7..59ce8c4 100644
> --- a/xen/common/argo.c
> +++ b/xen/common/argo.c
> @@ -37,6 +37,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_argo_addr_t);
>  DEFINE_XEN_GUEST_HANDLE(xen_argo_page_descr_t);
>  DEFINE_XEN_GUEST_HANDLE(xen_argo_register_ring_t);
>  DEFINE_XEN_GUEST_HANDLE(xen_argo_ring_t);
> +DEFINE_XEN_GUEST_HANDLE(xen_argo_unregister_ring_t);
>
>  /* Xen command line option to enable argo */
>  static bool __read_mostly opt_argo_enabled;
> @@ -666,6 +667,105 @@ ring_find_info(const struct domain *d, const struct 
> argo_ring_id *id)
>      return NULL;
>  }
>
> +static struct argo_send_info *
> +send_find_info(const struct domain *d, const struct argo_ring_id *id)
> +{
> +    struct hlist_node *node;
> +    struct argo_send_info *send_info;
> +
> +    hlist_for_each_entry(send_info, node, 
> &d->argo->send_hash[hash_index(id)],
> +                         node)
> +    {
> +        struct argo_ring_id *cmpid = &send_info->id;

Const.

> +
> +        if ( cmpid->port == id->port &&
> +             cmpid->domain_id == id->domain_id &&
> +             cmpid->partner_id == id->partner_id )
> +        {
> +            argo_dprintk("send_info=%p\n", send_info);
> +            return send_info;
> +        }
> +    }
> +    argo_dprintk("no send_info found\n");

Is this message actually helpful without printing any of the
parameters provided to the function?

> +
> +    return NULL;
> +}
> +
> +static long
> +unregister_ring(struct domain *currd,

Same as the comment made on the other patch, if this parameter is the
current domain there's no need to pass it around, or else it should be
named d instead of currd.

> +                XEN_GUEST_HANDLE_PARAM(xen_argo_unregister_ring_t) unreg_hnd)
> +{
> +    xen_argo_unregister_ring_t unreg;
> +    struct argo_ring_id ring_id;
> +    struct argo_ring_info *ring_info;
> +    struct argo_send_info *send_info;
> +    struct domain *dst_d = NULL;
> +    int ret;
> +
> +    ret = copy_from_guest(&unreg, unreg_hnd, 1) ? -EFAULT : 0;
> +    if ( ret )
> +        goto out;
> +
> +    ret = unreg.pad ? -EINVAL : 0;
> +    if ( ret )
> +        goto out;

I don't see the point in the out label when you could just use 'return
-EINVAL' or -EFAULT directly here and above.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel