Re: [PATCH 04/11] tools/xenstored: add GLOBAL_QUOTA_DATA record for live update

[Juergen Gross <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 6034 bytes --]

On 16.03.26 09:15, Jürgen Groß wrote:
> On 13.03.26 18:08, Anthony PERARD wrote:
>> On Thu, Mar 05, 2026 at 02:52:01PM +0100, Juergen Gross wrote:
>>> diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
>>> index acdcaa769e..694ae58973 100644
>>> --- a/tools/xenstored/domain.c
>>> +++ b/tools/xenstored/domain.c
>>> @@ -1332,6 +1332,27 @@ int do_set_feature(const void *ctx, struct connection 
>>> *conn,
>>>       return 0;
>>>   }
>>> +static bool parse_quota_name(const char *name, unsigned int *qidx,
>>> +                 unsigned int *idx)
>>> +{
>>> +    unsigned int q;
>>
>> What do you think of using something like:
>>      const char soft_prefix[] = "soft-";
>>      const size_t soft_prefix_len = sizeof(soft_prefix) - 1;
>> to explain the `5`, here and in e.g. the function build_quota_data() ?
>> We used this in libxl in one place:
>>      https://elixir.bootlin.com/xen/v4.21.0/source/tools/libs/light/ 
>> libxl_qmp.c#L1288
>>
>> But it's fine to leave it like that, as the '5's are close enought to
>> the prefix that we can guess easly enough.
> 
> I can change it, but I'd prefer to use macros for that purpose.
> 
>>
>>> +
>>> +    if (strncmp(name, "soft-", 5)) {
>>> +        *idx = Q_IDX_HARD;
>>> +    } else {
>>> +        *idx = Q_IDX_SOFT;
>>> +        name += 5;
>>> +    }
>>> +    for (q = 0; q < ACC_N; q++) {
>>> +        if (quota_adm[q].name && !strcmp(quota_adm[q].name, name)) {
>>> +            *qidx = q;
>>> +            return false;
>>> +        }
>>> +    }
>>> +
>>> +    return true;
>>> +}
>>> +
>>>   static int close_xgt_handle(void *_handle)
>>>   {
>>>       xengnttab_close(*(xengnttab_handle **)_handle);
>>> @@ -2001,6 +2022,61 @@ void read_state_connection(const void *ctx, const void 
>>> *state)
>>>       }
>>>   }
>>> +static unsigned int get_quota_size(struct quota *quota, unsigned int *len)
>>> +{
>>> +    unsigned int q;
>>> +    unsigned int n = 0;
>>> +
>>> +    for (q = 0; q < ACC_N; q++) {
>>> +        if (!quota_adm[q].name)
>>> +            continue;
>>> +        if (quota[q].val[Q_IDX_HARD] != Q_VAL_DISABLED) {
>>> +            n++;
>>> +            *len += strlen(quota_adm[q].name) + 1;
>>> +        }
>>> +        if (quota[q].val[Q_IDX_SOFT] != Q_VAL_DISABLED) {
>>> +            n++;
>>> +            *len += strlen(quota_adm[q].name) + 5 + 1;
>>
>> The value 5 here isn't explained. A comment would be nice.
> 
> Using the macro mentioned above will make it more descriptive.
> 
>>
>>> +        }
>>> +    }
>>> +
>>> +    return n;
>>> +}
>>> +
>>> +static void build_quota_data(struct quota *quota, uint32_t *val, char *name)
>>
>> I guess we will need a leap of faith to trust that `val` is big enough,
>> after finding out that it's actually an output of multiple values, and
>> not an input of a single value.
> 
> That's what get_quota_size() is calculating.
> 
>>
>> And `name` seems to also be an output, and this is actually impossible
>> to guess from the prototype.
> 
> True. What about names?

I have chosen names_buf instead, making it more clear that this is an
output parameter for multiple names.

> 
>>
>>> +{
>>> +    unsigned int q;
>>> +    unsigned int n = 0;
>>> +
>>> +    for (q = 0; q < ACC_N; q++) {
>>> +        if (!quota_adm[q].name)
>>> +            continue;
>>> +        if (quota[q].val[Q_IDX_HARD] != Q_VAL_DISABLED) {
>>> +            val[n++] = quota[q].val[Q_IDX_HARD];
>>> +            strcpy(name, quota_adm[q].name);
>>> +            name += strlen(name) + 1;
>>> +        }
>>> +        if (quota[q].val[Q_IDX_SOFT] != Q_VAL_DISABLED) {
>>> +            val[n++] = quota[q].val[Q_IDX_SOFT];
>>> +            strcpy(name, "soft-");
>>> +            strcpy(name + 5, quota_adm[q].name);
>>> +            name += strlen(name) + 1;
>>> +        }
>>> +    }
>>> +}
>>> +
>>> +static void parse_quota_data(const uint32_t *val, const char *name,
>>> +                 unsigned int n, struct quota *quota)
>>> +{
>>> +    unsigned int i, q, idx;
>>> +
>>> +    for (i = 0; i < n; i++) {
>>> +        if (!parse_quota_name(name, &q, &idx))
>>> +            quota[q].val[idx] = val[i];
>>> +        name += strlen(name) + 1;
>>
>> So for `val`, we have a size. But, we don't have a size for `name`, are
>> we sure that it's safe to keep reading past `NUL` characters ? Is the
>> size of name available somewhere?
> 
> Yes. get_quota_size() calculated that as well.
> 
>>
>>> +    }
>>> +}
>>> +
>>>   static int dump_state_domain(const void *k, void *v, void *arg)
>>>   {
>>>       struct domain *domain = v;
>>> @@ -2049,6 +2125,53 @@ void read_state_domain(const void *ctx, const void 
>>> *state, unsigned int version)
>>>           domain->features = sd->features;
>>>   }
>>> +const char *dump_state_glb_quota(FILE *fp)
>>> +{
>>> +    struct xs_state_record_header *head;
>>> +    struct xs_state_glb_quota *glb;
>>> +    void *record;
>>> +    unsigned int n_quota;
>>> +    unsigned int len = sizeof(*glb);
>>> +    size_t ret;
>>> +
>>> +    n_quota = get_quota_size(quotas, &len);
>>
>> So, get_quota_size is actually an "add" the size to this variable, and
>> not "store" the size in this variable. That's not confusing at all.
> 
> Would it be better if len is renamed to names_len (both here and the
> parameter of get_quota_size())?

In the end I have chosen to add a comment above get_quota_size() and
rename "len" to "rec_len" in dump_state_glb_quota().


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]