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