Re: [PATCH 08/20] tools/xenstore: add hashlist for finding struct domain by domid

[Julien Grall <julien@xxxxxxx>]

Hi Juergen,

On 01/11/2022 15:28, Juergen Gross wrote:
> @@ -341,49 +339,56 @@ static bool get_domain_info(unsigned int domid, 
> xc_dominfo_t *dominfo)
>                dominfo->domid == domid;
>   }
>   
> -void check_domains(void)
> +static int check_domain(void *k, void *v, void *arg)
Looking at this callback, shouldn't 'k' be const? If not, wouldn't this 
mean a caller could potentially mess up with the hashtable?
>   {
>         xc_dominfo_t dominfo;
> -       struct domain *domain;
>         struct connection *conn;
> -       int notify = 0;
>         bool dom_valid;
> +       struct domain *domain = v;
> +       bool *notify = arg;
>   
> - again:
> -       list_for_each_entry(domain, &domains, list) {
> -               dom_valid = get_domain_info(domain->domid, &dominfo);
> -               if (!domain->introduced) {
> -                       if (!dom_valid) {
> -                               talloc_free(domain);
> -                               goto again;
> -                       }
> -                       continue;
> -               }
> -               if (dom_valid) {
> -                       if ((dominfo.crashed || dominfo.shutdown)
> -                           && !domain->shutdown) {
> -                               domain->shutdown = true;
> -                               notify = 1;
> -                               /*
> -                                * Avoid triggering watch events when the
> -                                * domain's nodes are being deleted.
> -                                */
> -                               if (domain->conn)
> -                                       conn_delete_all_watches(domain->conn);
> -                       }
> -                       if (!dominfo.dying)
> -                               continue;
> +       dom_valid = get_domain_info(domain->domid, &dominfo);
> +       if (!domain->introduced) {
> +               if (!dom_valid) {
> +                       talloc_free(domain);
> +                       return 1;
You are only freeing one domain here. So shouldn't we return 0? If not 
please explain in a comment.
>                 }
> -               if (domain->conn) {
> -                       /* domain is a talloc child of domain->conn. */
> -                       conn = domain->conn;
> -                       domain->conn = NULL;
> -                       talloc_unlink(talloc_autofree_context(), conn);
> -                       notify = 0; /* destroy_domain() fires the watch */
> -                       goto again;
> +               return 0;
> +       }
> +       if (dom_valid) {
> +               if ((dominfo.crashed || dominfo.shutdown)
> +                   && !domain->shutdown) {
> +                       domain->shutdown = true;
> +                       *notify = true;
> +                       /*
> +                        * Avoid triggering watch events when the domain's
> +                        * nodes are being deleted.
> +                        */
> +                       if (domain->conn)
> +                               conn_delete_all_watches(domain->conn);
>                 }
> +               if (!dominfo.dying)
> +                       return 0;
> +       }
> +       if (domain->conn) {
> +               /* domain is a talloc child of domain->conn. */
> +               conn = domain->conn;
> +               domain->conn = NULL;
> +               talloc_unlink(talloc_autofree_context(), conn);
> +               *notify = false; /* destroy_domain() fires the watch */
> +               return 1;
Can you add a comment explaining why 1 is returned here? Is this because 
we may free two domains?
>         }
>   
> +	return 0;
> +}
Cheers,

--
Julien Grall