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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, May 06, 2014 at 01:45:30PM +0100, Ian Campbell wrote:
> 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).
> 

Good catch, thanks.

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

I'm not very sure about this. My thought was that not displaying the
name of the label is not critical, but on the other hand you can argue
that showing incorrect information is just wrong...

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

OK.

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

Right, it's just code motion.

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

I will check.

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

Correct. I can do a prerequisite patch to move this part around.

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

Thanks for the hint.

Wei.

> Ian.

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