Re: [PATCH 4/6] tools/libs: add a new libxenmanage library

[Jürgen Groß <jgross@xxxxxxxx>]

On 22.11.24 14:55, Anthony PERARD wrote:
> Hi Juergen,
>
> On Wed, Oct 23, 2024 at 03:10:03PM +0200, Juergen Gross wrote:
> > In order to have a stable interface in user land for using stable
> > domctl and possibly later sysctl interfaces, add a new library
> > libxenmanage.
> What this new library could do? What sort of operation could be added in
> the future? Domain creation? I'm trying to get convince that "manage" is
> the right name for it.
It is thought to encapsulate all (future) stable hypercalls replacing
current sysctl and domctl operations.

> To me, "manage" could be something higher level to take care of a domain
> from it's creation to its demise.
Yes, and to manage the host.

> So for this lib have get_domain_info() to query about a single domain,
> and get_changed_domain() which seems to be a state synchronisation
> operation. (For that second function, it resemble an operation of the
> Matrix API calling "https://.../sync"; which return all the new event
> since the last time it was called. But back to the new function name, a
> get* function which returns a different value every time you call it
> might not actually be the right name for it, maybe other functions that
> do something similar, or at least tell when there's a new event, would
> be poll() and select(), so maybe poll_changed_domain() would be slightly
> better at describing the kind of function that it is?)
Fine with me. I'm always in favor of descriptive names.

> So, those two functions only query about the states of domains, without
> making any modification is seems, so is "manage" still the right name?
> At least, it both function doesn't seems to fit in existing stable
> libraries so having a new one seems the right call. So the name
> depends of what other operation could be added to the library, as such,
> a description of the library would be nice, but at least thanks for
> documenting every functions!
I can add more comments, of course. This was just a first iteration to
get some feedback whether the general approach is okay.

> > diff --git a/tools/include/xenmanage.h b/tools/include/xenmanage.h
> > new file mode 100644
> > index 0000000000..2e6c3dedaa
> > --- /dev/null
> > +++ b/tools/include/xenmanage.h
> > @@ -0,0 +1,98 @@
> > +/*
> > + * Copyright (c) 2024 SUSE Software Solutions Germany GmbH
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation;
> > + * version 2.1 of the License.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library; If not, see <http://www.gnu.org/licenses/>.
> Shall we use SPDX tags instead of this boilerplate?
My thinking was to avoid that for "official" header files, as those might
be copied verbatim to other projects, which might not use SPDX. So having
the license text verbatim avoids any problems in this regard.

> > + */
> > +#ifndef XENMANAGE_H
> > +#define XENMANAGE_H
> > +
> > +#include <stdint.h>
> > +
> > +/* Callers who don't care don't need to #include <xentoollog.h> */
> > +struct xentoollog_logger;
> More like, callers who care will need to include xentoollog.h. Here, we
> just avoid the need to include xentoollog.h in xenmanage.h by declaring
> the minimum.
>
> If every time I wanted to include an header, I needed to figure which
> header needed to be include before, that would be very annoying. Often,
> headers include the needed headers.
>
> If you want to have a comment, how about "Avoid the need to include
> <xentoollog.h>", that way the comment tell why "struct
> xentoollog_logger" is here, where it came from, and is more explicit.
Okay.

> > diff --git a/tools/libs/manage/core.c b/tools/libs/manage/core.c
> > new file mode 100644
> > index 0000000000..0c9199f829
> > --- /dev/null
> > +++ b/tools/libs/manage/core.c
> > @@ -0,0 +1,170 @@
> ...
> > +
> > +#define __XEN_TOOLS__ 1
> This define might be better in the Makefile(.common), or even in libs.mk? So 
> far,
> only libxenhypfs does define this in source code, all the other defines
> are in CFLAGS or there because xenctrl.h is included.
Yes, that's better.

> > +static int xenmanage_do_domctl_get_domain_state(xenmanage_handle *hdl,
> > +                                                unsigned int domid_in,
> > +                                                unsigned int *domid_out,
> > +                                                unsigned int *state,
> > +                                                uint64_t *unique_id)
> > +{
> > +    struct xen_domctl *buf;
> > +    struct xen_domctl_get_domain_state *st;
> > +    int saved_errno;
> > +    int ret;
> > +
> ...
> > +
> > +    ret = xencall1(hdl->xcall, __HYPERVISOR_domctl, (unsigned long)buf);
> > +    saved_errno = errno;
> > +    if ( !ret )
> > +    {
> > +        st = &buf->u.get_domain_state;
> You could define *st here.
> struct xen_domctl_get_domain_state *st = &...;
>
> Or even with ".. *const st" but maybe that's not common enough in C
> code.
Okay, I'll move the definition down.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature