[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.