Re: [PATCH 1/5] tools/cpupools: Give a name to unnamed cpupools

[Luca Fancellu <luca.fancellu@xxxxxxx>]

> On 16 Feb 2022, at 06:13, Juergen Gross <jgross@xxxxxxxx> wrote:
> 
> On 15.02.22 18:48, Luca Fancellu wrote:
>>> On 15 Feb 2022, at 10:33, Juergen Gross <jgross@xxxxxxxx> wrote:
>>> 
>>> On 15.02.22 11:15, Luca Fancellu wrote:
>>>> With the introduction of boot time cpupools, Xen can create many
>>>> different cpupools at boot time other than cpupool with id 0.
>>>> Since these newly created cpupools can't have an
>>>> entry in Xenstore, create the entry using xen-init-dom0
>>>> helper with the usual convention: Pool-<cpupool id>.
>>>> Given the change, remove the check for poolid == 0 from
>>>> libxl_cpupoolid_to_name(...).
>>>> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
>>>> ---
>>>>  tools/helpers/xen-init-dom0.c  | 26 +++++++++++++++++++++++++-
>>>>  tools/libs/light/libxl_utils.c |  3 +--
>>>>  2 files changed, 26 insertions(+), 3 deletions(-)
>>>> diff --git a/tools/helpers/xen-init-dom0.c b/tools/helpers/xen-init-dom0.c
>>>> index c99224a4b607..3539f56faeb0 100644
>>>> --- a/tools/helpers/xen-init-dom0.c
>>>> +++ b/tools/helpers/xen-init-dom0.c
>>>> @@ -43,7 +43,10 @@ int main(int argc, char **argv)
>>>>      int rc;
>>>>      struct xs_handle *xsh = NULL;
>>>>      xc_interface *xch = NULL;
>>>> -    char *domname_string = NULL, *domid_string = NULL;
>>>> +    char *domname_string = NULL, *domid_string = NULL, *pool_string = 
>>>> NULL;
>> Hi Juergen,
>>> 
>>> pool_string seems to be unused.
>> I will remove it
>>> 
>>>> +    char pool_path[strlen("/local/pool") + 12], pool_name[strlen("Pool-") 
>>>> + 5];
>>> 
>>> I don't like that. Why don't you use pointers and ...
>>> 
>>>> +    xc_cpupoolinfo_t *xcinfo;
>>>> +    unsigned int pool_id = 0;
>>>>      libxl_uuid uuid;
>>>>        /* Accept 0 or 1 argument */
>>>> @@ -114,6 +117,27 @@ int main(int argc, char **argv)
>>>>          goto out;
>>>>      }
>>>>  +    /* Create an entry in xenstore for each cpupool on the system */
>>>> +    do {
>>>> +        xcinfo = xc_cpupool_getinfo(xch, pool_id);
>>>> +        if (xcinfo != NULL) {
>>>> +            if (xcinfo->cpupool_id != pool_id)
>>>> +                pool_id = xcinfo->cpupool_id;
>>>> +            snprintf(pool_path, sizeof(pool_path), "/local/pool/%d/name",
>>>> +                     pool_id);
>>>> +            snprintf(pool_name, sizeof(pool_name), "Pool-%d", pool_id);
>>> 
>>> ... use asprintf() here for allocating the strings in the needed size?
>> Why would you like more this approach? I was trying to avoid allocation/free
>> operations in a loop that would need also more code to check cases in which
>> memory is not allocated. Instead with the buffers in the stack we don’t have 
>> problems.
> 
> My main concerns are the usage of strlen() for sizing an on-stack array,
> the duplication of the format strings (once in the arrays definition and
> once in the snprintf()), and the mixture of strlen() and constants for
> sizing the arrays.
> 
> There are actually some errors in your approach for sizing the arrays,
> showing how fragile your solution is: you are allowing a "positive
> integer number" for a cpupool-id, which could easily need 10 digits,
> while your arrays allow only for 5 or 4 digits, depending on the array.
> 
> And doing the two asprintf() calls and then checking that both arrays
> are not NULL isn't that much code. BTW, your approach is missing the
> test that the arrays have been large enough.
> 
> The performance of that loop shouldn't be that critical that a few
> additional microseconds really hurt, especially as I don't think any
> use case will exceed single digit loop iterations.

Hi Juergen,

Thank you for your explanation, totally makes sense. I took inspiration from
libxl_cpupoolid_to_name in libxl_utils.c writing this code but I see the 
limitation
now.

I will change it to use asprintf().

Cheers,
Luca

> 
>>> 
>>>> +            pool_id++;
>>>> +            if (!xs_write(xsh, XBT_NULL, pool_path, pool_name,
>>>> +                          strlen(pool_name))) {
>>>> +                fprintf(stderr, "cannot set pool name\n");
>>>> +                rc = 1;
>>>> +            }
>>>> +            xc_cpupool_infofree(xch, xcinfo);
>>>> +            if (rc)
>>>> +                goto out;
>>> 
>>> Moving the call of xc_cpupool_infofree() ahead of the call of xs_write()
>>> would drop the need for this last if statement, as you could add the
>>> goto to the upper if.
>> Yes you are right, it would simplify the code
>>> 
>>>> +        }
>>>> +    } while(xcinfo != NULL);
>>>> +
>>> 
>>> With doing all of this for being able to assign other domains created
>>> at boot to cpupools, shouldn't you add names for other domains than dom0
>>> here, too?
>> This serie is more about cpupools, maybe I can do that in another commit out
>> of this serie.
> 
> Fine with me.
> 
> 
> Juergen
> <OpenPGP_0xB0DE9DD628BF132F.asc>