Re: [Xen-devel] [PATCH 3/6] libxl:xl: add parsing code to parse "libxl_static_sshm" from xl config files

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 22/08/17 21:36, Stefano Stabellini wrote:
> On Wed, 23 Aug 2017, Zhongze Liu wrote:
> > Add the parsing utils for the newly introduced libxl_static_sshm struct
> > to the libxl/libxlu_* family. And add realated parsing code in xl to
> > parse the struct from xl config files. This is for the proposal "Allow
> > setting up shared memory areas between VMs from xl config file" (see [1]).
> >
> > [1] https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg03047.html
> >
> > Signed-off-by: Zhongze Liu <blackskygg@xxxxxxxxx>
> >
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > Cc: Tim Deegan <tim@xxxxxxx>
> > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> > Cc: Julien Grall <julien.grall@xxxxxxx>
> > Cc: xen-devel@xxxxxxxxxxxxx
> > ---
> >  tools/libxl/Makefile      |   2 +-
> >  tools/libxl/libxlu_sshm.c | 210 ++++++++++++++++++++++++++++++++++++++++++++++
> >  tools/libxl/libxlutil.h   |   6 ++
> >  tools/xl/xl_parse.c       |  24 +++++-
> >  4 files changed, 240 insertions(+), 2 deletions(-)
> >  create mode 100644 tools/libxl/libxlu_sshm.c
> >
> > diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> > index 082af8f716..3b63fb2cad 100644
> > --- a/tools/libxl/Makefile
> > +++ b/tools/libxl/Makefile
> > @@ -175,7 +175,7 @@ AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h _libxl_list.h 
> > _paths.h \
> >  AUTOSRCS= libxlu_cfg_y.c libxlu_cfg_l.c
> >  AUTOSRCS += _libxl_save_msgs_callout.c _libxl_save_msgs_helper.c
> >  LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o \
> > -       libxlu_disk_l.o libxlu_disk.o libxlu_vif.o libxlu_pci.o
> > +       libxlu_disk_l.o libxlu_disk.o libxlu_vif.o libxlu_pci.o libxlu_sshm.o
> >  $(LIBXLU_OBJS): CFLAGS += $(CFLAGS_libxenctrl) # For xentoollog.h
> >
> >  $(TEST_PROG_OBJS) _libxl.api-for-check: CFLAGS += $(CFLAGS_libxentoollog)
> > diff --git a/tools/libxl/libxlu_sshm.c b/tools/libxl/libxlu_sshm.c
> > new file mode 100644
> > index 0000000000..8647665213
> > --- /dev/null
> > +++ b/tools/libxl/libxlu_sshm.c
> > @@ -0,0 +1,210 @@
> > +#include "libxl_osdeps.h" /* must come before any other headers */
> > +#include "libxlu_internal.h"
> > +
> > +#include <ctype.h>
> > +
> > +#define PARAM_RE(EXPR) "^\\s*" EXPR "\\s*(,|$)"
> > +#define WORD_RE         "([_a-zA-Z0-9]+)"
> > +#define EQU_RE         PARAM_RE(WORD_RE "\\s*=\\s*" WORD_RE)
> > +
> > +#define PAGE_SIZE_MASK ((uint64_t)0xfff)
> You can probably use XC_PAGE_MASK that is already defined?
> Otherwise I would name this XLU_PAGE_MASK and move the definition to one
> of the libxlu headers.
I am not maintaining this code, but could we please avoid introducing 
new definition of PAGE_MASK?
This would make more difficult to support an hypervisor using a 
different page granularity.
Furthermore, 0xfff is not obvious to read. It makes more sense to define 
it in term of PAGE_SIZE.
Cheers,

--
Julien Grall

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