On 20.12.22 21:15, Julien Grall wrote:
Hi,On 13/12/2022 16:00, Juergen Gross wrote:Rework the interface and the internals of the per-domain node accounting: - rename the functions to domain_nbentry_*() in order to better match the related counter name - switch from node pointer to domid as interface, as all nodes have the owner filled inThe downside is now you have may place open-coding "...->perms->p[0].id". IHMO this is making the code more complicated. So can you introduce a few wrappers that would take a node and then convert to the owner?
Okay.
- use a common internal function for adding a value to the counter For the transaction case add a helper function to get the list head of the per-transaction changed domains, enabling to eliminate the transaction_entry_*() functions. Signed-off-by: Juergen Gross <jgr...@suse.com> --- tools/xenstore/xenstored_core.c | 22 ++--- tools/xenstore/xenstored_domain.c | 122 +++++++++++-------------- tools/xenstore/xenstored_domain.h | 10 +- tools/xenstore/xenstored_transaction.c | 15 +-- tools/xenstore/xenstored_transaction.h | 7 +- 5 files changed, 72 insertions(+), 104 deletions(-) diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c index f96714e1b8..61569cecbb 100644 --- a/tools/xenstore/xenstored_core.c +++ b/tools/xenstore/xenstored_core.c@@ -1459,7 +1459,7 @@ static void destroy_node_rm(struct connection *conn, struct node *node)static int destroy_node(struct connection *conn, struct node *node) { destroy_node_rm(conn, node); - domain_entry_dec(conn, node); + domain_nbentry_dec(conn, node->perms.p[0].id); /* * It is not possible to easily revert the changes in a transaction.@@ -1498,7 +1498,7 @@ static struct node *create_node(struct connection *conn, const void *ctx,for (i = node; i; i = i->parent) { /* i->parent is set for each new node, so check quota. */ if (i->parent && - domain_entry(conn) >= quota_nb_entry_per_domain) { + domain_nbentry(conn) >= quota_nb_entry_per_domain) { ret = ENOSPC; goto err; }@@ -1509,7 +1509,7 @@ static struct node *create_node(struct connection *conn, const void *ctx,/* Account for new node */ if (i->parent) { - if (domain_entry_inc(conn, i)) { + if (domain_nbentry_inc(conn, i->perms.p[0].id)) { destroy_node_rm(conn, i); return NULL; }@@ -1662,7 +1662,7 @@ static int delnode_sub(const void *ctx, struct connection *conn,watch_exact = strcmp(root, node->name); fire_watches(conn, ctx, node->name, node, watch_exact, NULL); - domain_entry_dec(conn, node); + domain_nbentry_dec(conn, node->perms.p[0].id); return WALK_TREE_RM_CHILDENTRY; }@@ -1802,25 +1802,25 @@ static int do_set_perms(const void *ctx, struct connection *conn,return EPERM; old_perms = node->perms; - domain_entry_dec(conn, node); + domain_nbentry_dec(conn, node->perms.p[0].id); node->perms = perms; - if (domain_entry_inc(conn, node)) { + if (domain_nbentry_inc(conn, node->perms.p[0].id)) { node->perms = old_perms; /* * This should never fail because we had a reference on the * domain before and Xenstored is single-threaded. */ - domain_entry_inc(conn, node); + domain_nbentry_inc(conn, node->perms.p[0].id); return ENOMEM; } if (write_node(conn, node, false)) { int saved_errno = errno; - domain_entry_dec(conn, node); + domain_nbentry_dec(conn, node->perms.p[0].id); node->perms = old_perms; /* No failure possible as above. */ - domain_entry_inc(conn, node); + domain_nbentry_inc(conn, node->perms.p[0].id); errno = saved_errno; return errno; @@ -2392,7 +2392,7 @@ void setup_structure(bool live_update) manual_node("/tool/xenstored", NULL); manual_node("@releaseDomain", NULL); manual_node("@introduceDomain", NULL); - domain_entry_fix(dom0_domid, 5, true); + domain_nbentry_fix(dom0_domid, 5, true); } check_store(); @@ -3400,7 +3400,7 @@ void read_state_node(const void *ctx, const void *state) if (write_node_raw(NULL, &key, node, true)) barf("write node error restoring node"); - if (domain_entry_inc(&conn, node)) + if (domain_nbentry_inc(&conn, node->perms.p[0].id)) barf("node accounting error restoring node"); talloc_free(node);diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.cindex 3216119e83..40b24056c5 100644 --- a/tools/xenstore/xenstored_domain.c +++ b/tools/xenstore/xenstored_domain.c@@ -249,7 +249,7 @@ static int domain_tree_remove_sub(const void *ctx, struct connection *conn,domain->nbentry--; node->perms.p[0].id = priv_domid; node->acc.memory = 0; - domain_entry_inc(NULL, node); + domain_nbentry_inc(NULL, priv_domid); if (write_node_raw(NULL, &key, node, true)) { /* That's unfortunate. We only can try to continue. */ syslog(LOG_ERR, @@ -559,7 +559,7 @@ int acc_fix_domains(struct list_head *head, bool update) int cnt; list_for_each_entry(cd, head, list) { - cnt = domain_entry_fix(cd->domid, cd->nbentry, update); + cnt = domain_nbentry_fix(cd->domid, cd->nbentry, update); if (!update) { if (cnt >= quota_nb_entry_per_domain) return ENOSPC;@@ -604,18 +604,19 @@ static struct changed_domain *acc_get_changed_domain(const void *ctx,return cd; } -int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val, - unsigned int domid) +static int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val, + unsigned int domid) { struct changed_domain *cd; cd = acc_get_changed_domain(ctx, head, domid); if (!cd) - return errno; + return 0; + errno = 0; cd->nbentry += val; - return 0; + return cd->nbentry;You just introduced this helper in the previous patch (i.e. #9). So can you get the interface correct from the start? This will make easier to review the series.I don't mind too much if you add the static here. Although, it would have been nice if we avoid changing code just introduced.
Fine with me.
} static void domain_conn_reset(struct domain *domain) @@ -988,30 +989,6 @@ void domain_deinit(void) xenevtchn_unbind(xce_handle, virq_port); } -int domain_entry_inc(struct connection *conn, struct node *node) -{ - struct domain *d; - unsigned int domid; - - if (!node->perms.p) - return 0; - - domid = node->perms.p[0].id; - - if (conn && conn->transaction) { - transaction_entry_inc(conn->transaction, domid); - } else { - d = (conn && domid == conn->id && conn->domain) ? conn->domain - : find_or_alloc_existing_domain(domid); - if (d) - d->nbentry++; - else - return ENOMEM; - } - - return 0; -} - /* * Check whether a domain was created before or after a specific generation * count (used for testing whether a node permission is older than a domain). @@ -1079,62 +1056,67 @@ int domain_adjust_node_perms(struct node *node) return 0; } -void domain_entry_dec(struct connection *conn, struct node *node) +static int domain_nbentry_add(struct connection *conn, unsigned int domid, + int add, bool dom_exists)The name of the variable suggests that that if it is false then it doesn't exists. However, looking at how you use it, it is more a "Can struct domain be allocated?". So I would rename it to "dom_alloc_allowed" or similar.
I'll name it "no_dom_alloc".
{ struct domain *d; - unsigned int domid; - - if (!node->perms.p) - return; + struct list_head *head; + int ret; - domid = node->perms.p ? node->perms.p[0].id : conn->id; + if (conn && domid == conn->id && conn->domain) + d = conn->domain; + else if (dom_exists) { + d = find_domain_struct(domid); + if (!d) { + errno = ENOENT; + corrupt(conn, "Missing domain %u\n", domid); + return -1; + } + } else { + d = find_or_alloc_existing_domain(domid); + if (!d) { + errno = ENOMEM; + return -1; + } + } if (conn && conn->transaction) { - transaction_entry_dec(conn->transaction, domid); - } else { - d = (conn && domid == conn->id && conn->domain) ? conn->domain - : find_domain_struct(domid); - if (d) { - d->nbentry--; - } else { - errno = ENOENT; - corrupt(conn, - "Node \"%s\" owned by non-existing domain %u\n", - node->name, domid); + head = transaction_get_changed_domains(conn->transaction); + ret = acc_add_dom_nbentry(conn->transaction, head, add, domid); + if (errno) { + fail_transaction(conn->transaction); + return -1; } + return d->nbentry + ret;It is not entirely clear why you are return "d->nbentry + ret" here. If it is ...} + + d->nbentry += add; + + return d->nbentry; } -int domain_entry_fix(unsigned int domid, int num, bool update) +int domain_nbentry_inc(struct connection *conn, unsigned int domid) { - struct domain *d; - int cnt; + return (domain_nbentry_add(conn, domid, 1, false) < 0) ? errno : 0; +} - if (update) { - d = find_domain_struct(domid); - assert(d); - } else { - /* - * We are called first with update == false in order to catch - * any error. So do a possible allocation and check for error - * only in this case, as in the case of update == true nothing - * can go wrong anymore as the allocation already happened. - */ - d = find_or_alloc_existing_domain(domid); - if (!d) - return -1; - } +int domain_nbentry_dec(struct connection *conn, unsigned int domid) +{ + return (domain_nbentry_add(conn, domid, -1, true) < 0) ? errno : 0;... to make sure domain_nbentry_add() is not returning a negative value. Then it would not work.A good example imagine you have a transaction removing nodes from tree but not adding any. Then the "ret" would be negative.Meanwhile the nodes are also removed outside of the transaction. So the sum of "d->nbentry + ret" would be negative resulting to a failure here.
Thanks for catching this. I think the correct way to handle this is to return max(d->nbentry + ret, 0) in domain_nbentry_add(). The value might be imprecise, but always >= 0 and never wrong outside of a transaction collision.
Such change of behavior should pointed in the commit message. But then I am not convinced this should be part of this commit which is mainly reworking an interface (e.g. no functional change is expected).
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
