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

["Anthony PERARD" <anthony.perard@xxxxxxxxxx>]

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.

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

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?)

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!

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

> + */
> +#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.


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


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

Cheers,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech