Re: [PATCH v6] tools/xenstore: move xenstored sources into dedicated directory

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Mon, Aug 21, 2023 at 10:14:22AM +0200, Juergen Gross wrote:
> diff --git a/tools/xenstored/.gitignore b/tools/xenstored/.gitignore
> new file mode 100644
> index 0000000000..edbb5d79fe
> --- /dev/null
> +++ b/tools/xenstored/.gitignore
> @@ -0,0 +1 @@
> +xenstored

Could you write that "/xenstored" ? The prefix "/" just makes sure that
only the file in the current directory is ignored, and not any
"xenstored" in subdirectory. Just in case.

> diff --git a/tools/xenstored/Makefile b/tools/xenstored/Makefile
> new file mode 100644
> index 0000000000..f3bd3d43c4
> --- /dev/null
> +++ b/tools/xenstored/Makefile
> @@ -0,0 +1,48 @@
> +XEN_ROOT=$(CURDIR)/../..
> +include $(XEN_ROOT)/tools/Rules.mk
> +
> +include Makefile.common
> +
> +xenstored: LDLIBS += $(LDLIBS_libxenevtchn)
> +xenstored: LDLIBS += $(LDLIBS_libxengnttab)
> +xenstored: LDLIBS += $(LDLIBS_libxenctrl)
> +xenstored: LDLIBS += -lrt
> +xenstored: LDLIBS += $(SOCKET_LIBS)
> +
> +ifeq ($(CONFIG_SYSTEMD),y)
> +$(XENSTORED_OBJS-y): CFLAGS += $(SYSTEMD_CFLAGS)
> +xenstored: LDLIBS += $(SYSTEMD_LIBS)
> +endif
> +
> +TARGETS += xenstored

Could you change that to := instead of += ? TARGETS is currently
introduced with a := (in tools/xenstore/Makefile).

> diff --git a/tools/xs-clients/.gitignore b/tools/xs-clients/.gitignore
> new file mode 100644
> index 0000000000..233fd8228a
> --- /dev/null
> +++ b/tools/xs-clients/.gitignore
> @@ -0,0 +1,10 @@
> +xenstore
> +xenstore-chmod
> +xenstore-control
> +xenstore-exists
> +xenstore-list
> +xenstore-ls
> +xenstore-read
> +xenstore-rm
> +xenstore-watch
> +xenstore-write

Same thing here, could you prefix all those entries with "/"?

> diff --git a/tools/xenstore/Makefile b/tools/xs-clients/Makefile
> similarity index 74%
> rename from tools/xenstore/Makefile
> rename to tools/xs-clients/Makefile
> index dc39b6cb31..1c5740450a 100644
> --- a/tools/xenstore/Makefile
> +++ b/tools/xs-clients/Makefile

I'm tempted to ask for the targets "clients-install" and
"clients-uninstall" to be removed from this makefile. Nothing is calling
them in our build system and something outside the git tree that rely on
that would need to be adjusted to the new directory. But maybe that can
be done in a followup patch as it would help with reverting it if the
targets are actually useful.


In any case, the patch is already good:
Acked-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>

Thanks,

-- 
Anthony PERARD