Re: [PATCH 14/15] add a domU script to fetch overlays and applying them to linux

[Henry Wang <xin.wang2@xxxxxxx>]

Hi Jan,

On 4/25/2024 2:46 PM, Jan Beulich wrote:
> On 25.04.2024 02:54, Henry Wang wrote:
> > On 4/24/2024 2:16 PM, Jan Beulich wrote:
> > > On 24.04.2024 05:34, Henry Wang wrote:
> > > > From: Vikram Garhwal <fnu.vikram@xxxxxxxxxx>
> > > >
> > > > Introduce a shell script that runs in the background and calls
> > > > get_overlay to retrive overlays and add them (or remove them) to Linux
> > > > device tree (running as a domU).
> > > >
> > > > Signed-off-by: Vikram Garhwal <fnu.vikram@xxxxxxxxxx>
> > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> > > > Signed-off-by: Henry Wang <xin.wang2@xxxxxxx>
> > > > ---
> > > >    tools/helpers/Makefile       |  2 +-
> > > >    tools/helpers/get_overlay.sh | 81 ++++++++++++++++++++++++++++++++++++
> > > >    2 files changed, 82 insertions(+), 1 deletion(-)
> > > >    create mode 100755 tools/helpers/get_overlay.sh
> > > Besides the same naming issue as in the earlier patch, the script also
> > > looks very Linux-ish. Yet ...
> > I will fix the naming issue in v2. Would you mind elaborating a bit more
> > about the "Linux-ish" concern? I guess this is because the original use
> > case is on Linux, should I do anything about this?
> Well, the script won't work on other than Linux, will it? Therefore ...
>
> > > > --- a/tools/helpers/Makefile
> > > > +++ b/tools/helpers/Makefile
> > > > @@ -58,7 +58,6 @@ init-dom0less: $(INIT_DOM0LESS_OBJS)
> > > >    get_overlay: $(SHARE_OVERLAY_OBJS)
> > > >         $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenvchan) $(LDLIBS_libxenstore) 
> > > > $(LDLIBS_libxenctrl) $(LDLIBS_libxengnttab) $(APPEND_LDFLAGS)
> > > >    
> > > > -
> > > >    .PHONY: install
> > > >    install: all
> > > >         $(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
> > > > @@ -67,6 +66,7 @@ install: all
> > > >    .PHONY: uninstall
> > > >    uninstall:
> > > >         for i in $(TARGETS); do rm -f $(DESTDIR)$(LIBEXEC_BIN)/$$i; done
> > > > +       $(RM) $(DESTDIR)$(LIBEXEC_BIN)/get_overlay.sh
> > > >    
> > > >    .PHONY: clean
> > > >    clean:
> > > ... you touching only the uninstall target, it's not even clear to me
> > > how (and under what conditions) the script is going to make it into
> > > $(DESTDIR)$(LIBEXEC_BIN)/. Did you mean to add to $(TARGETS), perhaps,
> > > alongside the earlier added get-overlay binary?
> ... it first of needs to become clear under what conditions it is actually
> going to be installed.
>
> > You are right, I think the get-overlay binary and this script should be
> > installed if DTB overlay is supported. Checking the code, I found
> > LIBXL_HAVE_DT_OVERLAY which can indicate if we have this feature
> > supported in libxl. Do you think it is a good idea to use it to install
> > these two files in Makefile? Thanks.
> Counter question: If it's not going to be installed, how are people going
> to make use of it? If the script is intended for manual use only, I think
> that would want saying in the description. Yet then I couldn't see why
> the uninstall goal would need modifying.
Checking the code again, I feel like this is a mistake actually. I think 
this script should be installed together with the get-overlay 
application as the script actually calls get-overlay. The uninstall goal 
should remain untouched. I will fix this in v2.
> As to LIBXL_HAVE_DT_OVERLAY - that's not accessible from a Makefile, I
> guess?
Yes.

Kind regards,
Henry

> Jan