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

[Luca Fancellu <luca.fancellu@xxxxxxx>]

> 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.

> 
>> +            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.

Thanks for your review.

Cheers,
Luca

> 
> 
> Juergen
> <OpenPGP_0xB0DE9DD628BF132F.asc>