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

[Juergen Gross <jgross@xxxxxxxx>]

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.

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

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature