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

[Juergen Gross <jgross@xxxxxxxx>]

On 01.12.22 22:34, Julien Grall wrote:
> 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?
I have modified the previous patch to make k const. I hope you are
fine with me having kept your "Reviewed-by:".

> >   {
> >       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.
Oh, good catch. That was a leftover from a previous state where even
removing the entry itself wasn't allowed.

> >           }
> > -        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?
Should be the same case as above.

In the initial version I just mapped the removed "goto again" to a restart of
hashtable_iterate().


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature