[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 12/13] tools/xenstore: use generic accounting for remaining quotas
On 03.05.23 12:18, Julien Grall wrote:
On 05/04/2023 08:03, Juergen Gross wrote:
The maxrequests, node size, number of node permissions, and path length
quota are a little bit special, as they are either active in
transactions only (maxrequests), or they are just per item instead of
count values. Nevertheless being able to know the maximum number of
those quota related values per domain would be beneficial, so add them
to the generic accounting.
The per domain value will never show current numbers other than zero,
but the maximum number seen can be gathered the same way as the number
of nodes during a transaction.
To be able to use the const qualifier for a new function switch
domain_is_unprivileged() to take a const pointer, too.
Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
---
tools/xenstore/xenstored_core.c | 14 ++++-----
tools/xenstore/xenstored_core.h | 2 +-
tools/xenstore/xenstored_domain.c | 39 ++++++++++++++++++++------
tools/xenstore/xenstored_domain.h | 6 ++++
tools/xenstore/xenstored_transaction.c | 4 +--
tools/xenstore/xenstored_watch.c | 2 +-
6 files changed, 48 insertions(+), 19 deletions(-)
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 88c569b7d5..65df2866bf 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -799,8 +799,8 @@ int write_node_raw(struct connection *conn, TDB_DATA *key,
struct node *node,
+ node->perms.num * sizeof(node->perms.p[0])
+ node->datalen + node->childlen;
- if (!no_quota_check && domain_is_unprivileged(conn) &&
- data.dsize >= quota_max_entry_size) {
+ if (domain_max_chk(conn, ACC_NODESZ, data.dsize, quota_max_entry_size)
+ && !no_quota_check) {
It feels a bit odd to move the !no_quota_check right after the actual check. But
AFAICT, you are doing it because domain_max_chk() will also update the maximum
value seen by the current quota.
Correct.
Is that correct? If so, it would be worth mentioning it in a comment.
Okay.
errno = ENOSPC;
return errno;
}
@@ -1168,7 +1168,7 @@ static bool valid_chars(const char *node)
"0123456789-/_@") == strlen(node));
}
-bool is_valid_nodename(const char *node)
+bool is_valid_nodename(const struct connection *conn, const char *node)
{
int local_off = 0;
unsigned int domid;
@@ -1188,7 +1188,8 @@ bool is_valid_nodename(const char *node)
if (sscanf(node, "/local/domain/%5u/%n", &domid, &local_off) != 1)
local_off = 0;
- if (strlen(node) > local_off + quota_max_path_len)
+ if (domain_max_chk(conn, ACC_PATHLEN, strlen(node) - local_off,
+ quota_max_path_len))
return false;
return valid_chars(node);
@@ -1250,7 +1251,7 @@ static struct node *get_node_canonicalized(struct
connection *conn,
*canonical_name = canonicalize(conn, ctx, name);
if (!*canonical_name)
return NULL;
- if (!is_valid_nodename(*canonical_name)) {
+ if (!is_valid_nodename(conn, *canonical_name)) {
errno = EINVAL;
return NULL;
}
@@ -1775,8 +1776,7 @@ static int do_set_perms(const void *ctx, struct
connection *conn,
return EINVAL;
perms.num--;
- if (domain_is_unprivileged(conn) &&
- perms.num > quota_nb_perms_per_node)
+ if (domain_max_chk(conn, ACC_NPERM, perms.num, quota_nb_perms_per_node))
return ENOSPC;
permstr = in->buffer + strlen(in->buffer) + 1;
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index 3564d85d7d..9339820156 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -258,7 +258,7 @@ void check_store(void);
void corrupt(struct connection *conn, const char *fmt, ...);
/* Is this a valid node name? */
-bool is_valid_nodename(const char *node);
+bool is_valid_nodename(const struct connection *conn, const char *node);
/* Get name of parent node. */
char *get_parent(const void *ctx, const char *node);
diff --git a/tools/xenstore/xenstored_domain.c
b/tools/xenstore/xenstored_domain.c
index d21f31da92..49e2c5c82a 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -433,7 +433,7 @@ int domain_get_quota(const void *ctx, struct connection
*conn,
return ENOMEM;
#define ent(t, e) \
- resp = talloc_asprintf_append(resp, "%-16s: %8u (max: %8u\n", #t, \
+ resp = talloc_asprintf_append(resp, "%-17s: %8u (max: %8u\n", #t, \
This changes feels a bit unrelated. Can you mention why this is necessary in the
commit message?
"transaction-nodes" has 17 characters. :-)
d->acc[e].val, d->acc[e].max); \
if (!resp) return ENOMEM
@@ -442,6 +442,7 @@ int domain_get_quota(const void *ctx, struct connection
*conn,
ent(transactions, ACC_TRANS);
ent(outstanding, ACC_OUTST);
ent(memory, ACC_MEM);
+ ent(transaction-nodes, ACC_TRANSNODES);
You seem to convert multiple quotas but only print one. Why?
Ah, sorry for omitting the other ones. The following patch is adding
them again, so I didn't recognize them missing.
#undef ent
@@ -459,7 +460,7 @@ int domain_max_global_acc(const void *ctx, struct
connection *conn)
return ENOMEM;
#define ent(t, e) \
- resp = talloc_asprintf_append(resp, "%-16s: %8u\n", #t, \
+ resp = talloc_asprintf_append(resp, "%-17s: %8u\n", #t, \
acc_global_max[e]); \
Ditto.
if (!resp) return ENOMEM
@@ -468,6 +469,7 @@ int domain_max_global_acc(const void *ctx, struct
connection *conn)
ent(transactions, ACC_TRANS);
ent(outstanding, ACC_OUTST);
ent(memory, ACC_MEM);
+ ent(transaction-nodes, ACC_TRANSNODES);
#undef ent
@@ -1081,12 +1083,22 @@ int domain_adjust_node_perms(struct node *node)
return 0;
}
+static void domain_acc_valid_max(struct domain *d, enum accitem what,
+ unsigned int val)
+{
+ assert(what < ARRAY_SIZE(d->acc));
+ assert(what < ARRAY_SIZE(acc_global_max));
+
+ if (val > d->acc[what].max)
+ d->acc[what].max = val;
+ if (val > acc_global_max[what] && domid_is_unprivileged(d->domid))
+ acc_global_max[what] = val;
+}
+
static int domain_acc_add_valid(struct domain *d, enum accitem what, int add)
{
unsigned int val;
- assert(what < ARRAY_SIZE(d->acc));
I think this assert should be kept because...
-
if ((add < 0 && -add > d->acc[what].val) ||
... of this check. Otherwise, you would check that 'what' is within the bounds
after the use.
Okay.
Juergen
Attachment:
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
|
