Re: [Xen-devel] [PATCH V4 01/24] xl / libxl: push parsing of SSID and CPU pool ID down to libxl

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2014-05-01 at 13:57 +0100, Wei Liu wrote:
> This patch pushes parsing of "init_seclabel", "seclabel",
> "device_model_stubdomain_seclabel" and "pool" down to libxl level.
> 
> Originally the parsing is done in xl level, which is not ideal because
> libxl won't have the truely relavent information. Now libxl holds
> important information by itself. This is useful when we do serialization
> and deserialization of domain configurations on libxl level.
> 
> The libxl IDL is extended to hold the string of labels and pool name.
> And if there those strings are present they take precedence over the
> numeric representations.
> 
> As all relavent structures have a field called X_name / X_label now, a

"relevant" (several of these).

> string is also copied there so that we can use it directly.
> 
> In order to be compatible with users of older versions of libxl, this
> patch also defines LIBXL_HAVE_SSID_AND_CPUPOOLID_PARSING. If it is
> defined, those strings are available. And if those strings are not NULL,
> libxl will do the parsing and ignore the numeric values.
> 
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> Cc: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> Cc: Juergen Gross <juergen.gross@xxxxxxxxxxxxxx>
> Cc: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
> ---
>  tools/libxl/libxl.c         |   19 ++++--
>  tools/libxl/libxl.h         |   12 ++++
>  tools/libxl/libxl_create.c  |   57 ++++++++++++++++
>  tools/libxl/libxl_dm.c      |    4 ++
>  tools/libxl/libxl_types.idl |    6 ++
>  tools/libxl/libxl_utils.c   |   31 +++++++++
>  tools/libxl/libxl_utils.h   |    3 +
>  tools/libxl/xl_cmdimpl.c    |  150 
> ++++++++++---------------------------------
>  tools/libxl/xl_sxp.c        |    7 +-
>  9 files changed, 164 insertions(+), 125 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 30b0b06..9a90f40 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -520,12 +520,18 @@ int libxl_domain_preserve(libxl_ctx *ctx, uint32_t 
> domid,
>      return 0;
>  }
>  
> -static void xcinfo2xlinfo(const xc_domaininfo_t *xcinfo,
> +static void xcinfo2xlinfo(libxl_ctx *ctx,
> +                          const xc_domaininfo_t *xcinfo,
>                            libxl_dominfo *xlinfo)
>  {
> +    size_t size;
> +
>      memcpy(&(xlinfo->uuid), xcinfo->handle, sizeof(xen_domain_handle_t));
>      xlinfo->domid = xcinfo->domain;
>      xlinfo->ssidref = xcinfo->ssidref;
> +    if (libxl_flask_sid_to_context(ctx, xlinfo->ssidref,
> +                                   &xlinfo->ssid_label, &size) < 0)
> +        xlinfo->ssid_label = NULL;

Is this a critical error?

> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index b2c3015..2f14d26 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -462,6 +462,18 @@
>  #define LIBXL_EXTERNAL_CALLERS_ONLY /* disappears for callers outside libxl 
> */
>  #endif
>  
> +/*
> + * LIBXL_HAVE_SSID_AND_CPUPOOLID_PARSING
> + *
> + * If this is defined, then libxl IDL contains strings of XSM security
> + * labels and CPU pool name.
> + *
> + * If those strings are not NULL, libxl will overwrite the numeric
> + * representations of these configurations regardless if they've been
> + * set by the caller, because libxl will do the parsing by itself.

Can you reference the struct and field names explicitly please.

I think it would be fine to have 2 #defines here and a more conventional
name would be LIBXL_HAVE_<STRUCT>_<FIELD>, which splitting would allow.

> + */
> +#define LIBXL_HAVE_SSID_AND_CPUPOOLID_PARSING 1
> +
>  typedef uint8_t libxl_mac[6];
>  #define LIBXL_MAC_FMT "%02hhx:%02hhx:%02hhx:%02hhx:%02hhx:%02hhx"
>  #define LIBXL_MAC_FMTLEN ((2*6)+5) /* 6 hex bytes plus 5 colons */
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index d015cf4..fe3bdd2 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -724,6 +724,63 @@ static void initiate_domain_create(libxl__egc *egc,
>  
>      domid = 0;

I guess this is just code motion, so I'm happy with it going in, even if
the error behaviour of libxl_flask_context_to_size strikes me as a bit
odd (libxl doesn't normally use errno).
 
> +    if (d_config->c_info.ssid_label) {
> +        char *s = d_config->c_info.ssid_label;
> +        ret = libxl_flask_context_to_sid(ctx, s, strlen(s),
> +                                         &d_config->c_info.ssidref);
> +        if (ret) {
> +            if (errno == ENOSYS) {
> +                LOG(WARN, "XSM Disabled: init_seclabel not supported");
> +                ret = 0;
> +            } else {
> +                LOG(ERROR, "Invalid init_seclabel: %s", s);
> +                goto error_out;
> +            }
> +        }
> +    }

> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 0f7bbf8..5f4341f 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -215,6 +215,7 @@ libxl_dominfo = Struct("dominfo",[
>      ("uuid",        libxl_uuid),
>      ("domid",       libxl_domid),
>      ("ssidref",     uint32),
> +    ("ssid_label", string),

Alignment (or perhaps hard/soft tabs?) There were a few of these.

> diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
> index 1f334f2..476921e 100644
> --- a/tools/libxl/libxl_utils.c
> +++ b/tools/libxl/libxl_utils.c

All just code motion? (Can the code motion aspect here be easily split
out?)

[...]
> +    if (!xlu_cfg_get_string (config, "init_seclabel", &buf, 0))
> +        c_info->ssid_label = strdup(buf);

You might find xlu_cfg_replace_string useful at various points here.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel