> On 16 Feb 2022, at 06:13, Juergen Gross <jgr...@suse.com> wrote:
>
> On 15.02.22 18:48, Luca Fancellu wrote:
>>> On 15 Feb 2022, at 10:33, Juergen Gross <jgr...@suse.com> 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.fance...@arm.com>
>>>> ---
>>>> 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>
