[PATCH v1 0/7] tools/ocaml code and build cleanups

[PATCH v1 0/7] tools/ocaml code and build cleanups

[Edwin Török]

Various OCaml code cleanups to make building and working on Oxenstored easier,
including compatibility with newer language versions.
This does not yet change the minimum version of OCaml.

A version of this series in a git repository is publicly available at:
https://github.com/edwintorok/xen.git
https://github.com/edwintorok/xen/compare/private/edvint/public?expand=1

Edwin Török (7):
  tools/ocaml/Makefile: do not run ocamldep during make clean
  tools/ocaml/*/Makefile: generate paths.ml from configure
  tools/ocaml/*/dune: dune based build system
  tools/ocaml: Makefile to drive dune
  tools/ocaml: fix compiler warnings
  tools/ocaml/libs/xb: hide type of Xb.t
  tools/ocaml/libs/eventchn: do not leak event channels and OCaml 5.0
    compat

 Makefile                                      |  5 ++
 tools/.gitignore                              |  7 ++
 tools/configure                               |  4 +-
 tools/configure.ac                            |  2 +
 tools/dune                                    |  5 ++
 tools/dune-project                            |  1 +
 tools/ocaml/Makefile.dune                     | 88 +++++++++++++++++++
 tools/ocaml/Makefile.rules                    |  2 +
 tools/ocaml/dune-project                      | 27 ++++++
 tools/ocaml/dune-workspace.dev.in             |  2 +
 tools/ocaml/dune-workspace.in                 | 18 ++++
 tools/ocaml/libs/eventchn/dune                | 11 +++
 tools/ocaml/libs/eventchn/xeneventchn_stubs.c | 29 +++++-
 tools/ocaml/libs/mmap/dune                    |  9 ++
 tools/ocaml/libs/xb/dune                      | 10 +++
 tools/ocaml/libs/xb/xb.ml                     |  3 +
 tools/ocaml/libs/xb/xb.mli                    |  9 +-
 tools/ocaml/libs/xc/dune                      | 16 ++++
 tools/ocaml/libs/xs/Makefile                  |  5 --
 tools/ocaml/libs/xs/dune                      | 15 ++++
 tools/ocaml/libs/xs/paths.ml.in               |  1 +
 tools/ocaml/xenstored/Makefile                |  5 --
 tools/ocaml/xenstored/connection.ml           | 10 +--
 tools/ocaml/xenstored/dune                    | 51 +++++++++++
 tools/ocaml/xenstored/paths.ml.in             |  4 +
 tools/ocaml/xenstored/process.ml              |  5 +-
 26 files changed, 315 insertions(+), 29 deletions(-)
 create mode 100644 tools/.gitignore
 create mode 100644 tools/dune
 create mode 100644 tools/dune-project
 create mode 100644 tools/ocaml/Makefile.dune
 create mode 100644 tools/ocaml/dune-project
 create mode 100644 tools/ocaml/dune-workspace.dev.in
 create mode 100644 tools/ocaml/dune-workspace.in
 create mode 100644 tools/ocaml/libs/eventchn/dune
 create mode 100644 tools/ocaml/libs/mmap/dune
 create mode 100644 tools/ocaml/libs/xb/dune
 create mode 100644 tools/ocaml/libs/xc/dune
 create mode 100644 tools/ocaml/libs/xs/dune
 create mode 100644 tools/ocaml/libs/xs/paths.ml.in
 create mode 100644 tools/ocaml/xenstored/dune
 create mode 100644 tools/ocaml/xenstored/paths.ml.in

-- 
2.34.1

[PATCH v1 1/7] tools/ocaml/Makefile: do not run ocamldep during make clean

[Edwin Török]

Trying to include .ocamldep.make will cause it to be generated if it
doesn't exist.
We do not want this during make clean: we would remove it anyway.

Speeds up make clean.

Before (measured on f732240fd3bac25116151db5ddeb7203b62e85ce, July 2022):
```
Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl

 Performance counter stats for 'make clean -j8 -s' (5 runs):

            4.2233 +- 0.0208 seconds time elapsed  ( +-  0.49% )
```

After:
```
perf stat -r 5 --null make clean -j8 -s

 Performance counter stats for 'make clean -j8 -s' (5 runs):

            2.7325 +- 0.0138 seconds time elapsed  ( +-  0.51% )
```

No functional change.

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
---
 tools/ocaml/Makefile.rules | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/ocaml/Makefile.rules b/tools/ocaml/Makefile.rules
index 7e4db457a1..d368308d9b 100644
--- a/tools/ocaml/Makefile.rules
+++ b/tools/ocaml/Makefile.rules
@@ -44,8 +44,10 @@ META: META.in
 
 ALL_OCAML_OBJ_SOURCES=$(addsuffix .ml, $(ALL_OCAML_OBJS))
 
+ifneq ($(MAKECMDGOALS),clean)
 .ocamldep.make: $(ALL_OCAML_OBJ_SOURCES) Makefile $(OCAML_TOPLEVEL)/Makefile.rules
 	$(call quiet-command, $(OCAMLDEP) $(ALL_OCAML_OBJ_SOURCES) *.mli $o,MLDEP,)
+endif
 
 clean: $(CLEAN_HOOKS)
 	$(Q)rm -f .*.d *.o *.so *.a *.cmo *.cmi *.cma *.cmx *.cmxa *.annot *.spot *.spit $(LIBS) $(PROGRAMS) $(GENERATED_FILES) .ocamldep.make META
-- 
2.34.1

Re: [PATCH v1 1/7] tools/ocaml/Makefile: do not run ocamldep during make clean

[Christian Lindig]

[-- Attachment #1: Type: text/plain, Size: 386 bytes --]



On 29 Jul 2022, at 18:53, Edwin Török <edvin.torok@citrix.com<mailto:edvin.torok@citrix.com>> wrote:

Trying to include .ocamldep.make will cause it to be generated if it
doesn't exist.
We do not want this during make clean: we would remove it anyway.

Speeds up make clean.

Acked-by: Christian Lindig <christian.lindig@citrix.com<mailto:christian.lindig@citrix.com>>


[-- Attachment #2: Type: text/html, Size: 3782 bytes --]

Re: [PATCH v1 1/7] tools/ocaml/Makefile: do not run ocamldep during make clean

[Jan Beulich]

On 29.07.2022 19:53, Edwin Török wrote:
> Trying to include .ocamldep.make will cause it to be generated if it
> doesn't exist.
> We do not want this during make clean: we would remove it anyway.
> 
> Speeds up make clean.
> 
> Before (measured on f732240fd3bac25116151db5ddeb7203b62e85ce, July 2022):
> ```
> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
> 
>  Performance counter stats for 'make clean -j8 -s' (5 runs):
> 
>             4.2233 +- 0.0208 seconds time elapsed  ( +-  0.49% )
> ```
> 
> After:
> ```
> perf stat -r 5 --null make clean -j8 -s
> 
>  Performance counter stats for 'make clean -j8 -s' (5 runs):
> 
>             2.7325 +- 0.0138 seconds time elapsed  ( +-  0.51% )
> ```
> 
> No functional change.
> 
> Signed-off-by: Edwin Török <edvin.torok@citrix.com>

I've committed this as is since it was acked and is an improvement
in any event, but ...

> --- a/tools/ocaml/Makefile.rules
> +++ b/tools/ocaml/Makefile.rules
> @@ -44,8 +44,10 @@ META: META.in
>  
>  ALL_OCAML_OBJ_SOURCES=$(addsuffix .ml, $(ALL_OCAML_OBJS))
>  
> +ifneq ($(MAKECMDGOALS),clean)
>  .ocamldep.make: $(ALL_OCAML_OBJ_SOURCES) Makefile $(OCAML_TOPLEVEL)/Makefile.rules
>  	$(call quiet-command, $(OCAMLDEP) $(ALL_OCAML_OBJ_SOURCES) *.mli $o,MLDEP,)
> +endif

... what about the distclean goal?

Jan

Re: [PATCH v1 1/7] tools/ocaml/Makefile: do not run ocamldep during make clean

[Edwin Torok]

> On 3 Aug 2022, at 11:16, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 29.07.2022 19:53, Edwin Török wrote:
>> Trying to include .ocamldep.make will cause it to be generated if it
>> doesn't exist.
>> We do not want this during make clean: we would remove it anyway.
>> 
>> Speeds up make clean.
>> 
>> Before (measured on f732240fd3bac25116151db5ddeb7203b62e85ce, July 2022):
>> ```
>> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
>> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
>> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
>> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
>> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
>> 
>> Performance counter stats for 'make clean -j8 -s' (5 runs):
>> 
>>            4.2233 +- 0.0208 seconds time elapsed  ( +-  0.49% )
>> ```
>> 
>> After:
>> ```
>> perf stat -r 5 --null make clean -j8 -s
>> 
>> Performance counter stats for 'make clean -j8 -s' (5 runs):
>> 
>>            2.7325 +- 0.0138 seconds time elapsed  ( +-  0.51% )
>> ```
>> 
>> No functional change.
>> 
>> Signed-off-by: Edwin Török <edvin.torok@citrix.com>
> 
> I've committed this as is since it was acked and is an improvement
> in any event, but ...
> 
>> --- a/tools/ocaml/Makefile.rules
>> +++ b/tools/ocaml/Makefile.rules
>> @@ -44,8 +44,10 @@ META: META.in
>> 
>> ALL_OCAML_OBJ_SOURCES=$(addsuffix .ml, $(ALL_OCAML_OBJS))
>> 
>> +ifneq ($(MAKECMDGOALS),clean)
>> .ocamldep.make: $(ALL_OCAML_OBJ_SOURCES) Makefile $(OCAML_TOPLEVEL)/Makefile.rules
>> 	$(call quiet-command, $(OCAMLDEP) $(ALL_OCAML_OBJ_SOURCES) *.mli $o,MLDEP,)
>> +endif
> 
> ... what about the distclean goal?


Thanks for the suggestion, I see other Makefiles using 'findstring clean', would that be appropriate here?

Something like this perhaps?

From 53cbf81a9c7d5090443b5fe5de37a47118ac53d8 Mon Sep 17 00:00:00 2001
Message-Id: <53cbf81a9c7d5090443b5fe5de37a47118ac53d8.1659522178.git.edvin.torok@citrix.com>
From: =?UTF-8?q?Edwin=20T=C3=B6r=C3=B6k?= <edvin.torok@citrix.com>
To: xen-devel@lists.xenproject.org
Cc: Christian Lindig <christian.lindig@citrix.com>
Cc: David Scott <dave@recoil.org>
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
Date: Wed, 3 Aug 2022 11:21:46 +0100
Subject: [PATCH] tools/ocaml/Makefile.rules: do not run ocamldep on distclean
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
---
 tools/ocaml/Makefile.rules | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/ocaml/Makefile.rules b/tools/ocaml/Makefile.rules
index d368308d9b..c1c5dd3b39 100644
--- a/tools/ocaml/Makefile.rules
+++ b/tools/ocaml/Makefile.rules
@@ -44,7 +44,7 @@ META: META.in

 ALL_OCAML_OBJ_SOURCES=$(addsuffix .ml, $(ALL_OCAML_OBJS))

-ifneq ($(MAKECMDGOALS),clean)
+ifeq (,$(findstring clean,$(MAKECMDGOALS)))
 .ocamldep.make: $(ALL_OCAML_OBJ_SOURCES) Makefile $(OCAML_TOPLEVEL)/Makefile.rules
 	$(call quiet-command, $(OCAMLDEP) $(ALL_OCAML_OBJ_SOURCES) *.mli $o,MLDEP,)
 endif
--
2.34.1

Re: [PATCH v1 1/7] tools/ocaml/Makefile: do not run ocamldep during make clean

[Jan Beulich]

On 03.08.2022 12:24, Edwin Torok wrote:
> 
> 
>> On 3 Aug 2022, at 11:16, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 29.07.2022 19:53, Edwin Török wrote:
>>> Trying to include .ocamldep.make will cause it to be generated if it
>>> doesn't exist.
>>> We do not want this during make clean: we would remove it anyway.
>>>
>>> Speeds up make clean.
>>>
>>> Before (measured on f732240fd3bac25116151db5ddeb7203b62e85ce, July 2022):
>>> ```
>>> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
>>> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
>>> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
>>> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
>>> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
>>>
>>> Performance counter stats for 'make clean -j8 -s' (5 runs):
>>>
>>>            4.2233 +- 0.0208 seconds time elapsed  ( +-  0.49% )
>>> ```
>>>
>>> After:
>>> ```
>>> perf stat -r 5 --null make clean -j8 -s
>>>
>>> Performance counter stats for 'make clean -j8 -s' (5 runs):
>>>
>>>            2.7325 +- 0.0138 seconds time elapsed  ( +-  0.51% )
>>> ```
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Edwin Török <edvin.torok@citrix.com>
>>
>> I've committed this as is since it was acked and is an improvement
>> in any event, but ...
>>
>>> --- a/tools/ocaml/Makefile.rules
>>> +++ b/tools/ocaml/Makefile.rules
>>> @@ -44,8 +44,10 @@ META: META.in
>>>
>>> ALL_OCAML_OBJ_SOURCES=$(addsuffix .ml, $(ALL_OCAML_OBJS))
>>>
>>> +ifneq ($(MAKECMDGOALS),clean)
>>> .ocamldep.make: $(ALL_OCAML_OBJ_SOURCES) Makefile $(OCAML_TOPLEVEL)/Makefile.rules
>>> 	$(call quiet-command, $(OCAMLDEP) $(ALL_OCAML_OBJ_SOURCES) *.mli $o,MLDEP,)
>>> +endif
>>
>> ... what about the distclean goal?
> 
> 
> Thanks for the suggestion, I see other Makefiles using 'findstring clean', would that be appropriate here?

Hmm, not sure this couldn't end up suppressing the rul when it's
needed. How about "ifeq ($(filter-out %clean,$(MAKECMDGOALS)),)"?
(Off the top of my head I don't recall whether this may additionally
need wrapping in $(strip ...).)

Jan

Re: [PATCH v1 1/7] tools/ocaml/Makefile: do not run ocamldep during make clean

[Anthony PERARD]

On Wed, Aug 03, 2022 at 10:24:26AM +0000, Edwin Torok wrote:
> 
> -ifneq ($(MAKECMDGOALS),clean)
> +ifeq (,$(findstring clean,$(MAKECMDGOALS)))

I think it would be better with $(filter-out,):

    ifeq (,$(filter-out %clean,$(MAKECMDGOALS)))

>  .ocamldep.make: $(ALL_OCAML_OBJ_SOURCES) Makefile $(OCAML_TOPLEVEL)/Makefile.rules
>  	$(call quiet-command, $(OCAMLDEP) $(ALL_OCAML_OBJ_SOURCES) *.mli $o,MLDEP,)

Also, don't hide this rule, instead, hide the "-include", there is no
need to have make waist time trying to find a rule to make
".ocamldep.make" and failing when not needed.

>  endif
> --
> 2.34.1

-- 
Anthony PERARD

Re: [PATCH v1 1/7] tools/ocaml/Makefile: do not run ocamldep during make clean

[Jan Beulich]

On 03.08.2022 12:57, Anthony PERARD wrote:
> On Wed, Aug 03, 2022 at 10:24:26AM +0000, Edwin Torok wrote:
>>
>> -ifneq ($(MAKECMDGOALS),clean)
>> +ifeq (,$(findstring clean,$(MAKECMDGOALS)))
> 
> I think it would be better with $(filter-out,):
> 
>     ifeq (,$(filter-out %clean,$(MAKECMDGOALS)))
> 
>>  .ocamldep.make: $(ALL_OCAML_OBJ_SOURCES) Makefile $(OCAML_TOPLEVEL)/Makefile.rules
>>  	$(call quiet-command, $(OCAMLDEP) $(ALL_OCAML_OBJ_SOURCES) *.mli $o,MLDEP,)
> 
> Also, don't hide this rule, instead, hide the "-include", there is no
> need to have make waist time trying to find a rule to make
> ".ocamldep.make" and failing when not needed.

Hmm, this sounds like I should be reverting the commit?

Jan

Re: [PATCH v1 1/7] tools/ocaml/Makefile: do not run ocamldep during make clean

[Anthony PERARD]

On Wed, Aug 03, 2022 at 01:58:34PM +0200, Jan Beulich wrote:
> On 03.08.2022 12:57, Anthony PERARD wrote:
> > On Wed, Aug 03, 2022 at 10:24:26AM +0000, Edwin Torok wrote:
> >>
> >> -ifneq ($(MAKECMDGOALS),clean)
> >> +ifeq (,$(findstring clean,$(MAKECMDGOALS)))
> > 
> > I think it would be better with $(filter-out,):
> > 
> >     ifeq (,$(filter-out %clean,$(MAKECMDGOALS)))
> > 
> >>  .ocamldep.make: $(ALL_OCAML_OBJ_SOURCES) Makefile $(OCAML_TOPLEVEL)/Makefile.rules
> >>  	$(call quiet-command, $(OCAMLDEP) $(ALL_OCAML_OBJ_SOURCES) *.mli $o,MLDEP,)
> > 
> > Also, don't hide this rule, instead, hide the "-include", there is no
> > need to have make waist time trying to find a rule to make
> > ".ocamldep.make" and failing when not needed.
> 
> Hmm, this sounds like I should be reverting the commit?

Well, it's not exactly an issue as there isn't any alternative rules,
and make is told to ignore failures to make ".ocamldep.make"; so `make
clean` and other targets still works as expected. Just a follow-up patch
would be fine I think.

-- 
Anthony PERARD

[PATCH v1 2/7] tools/ocaml/*/Makefile: generate paths.ml from configure

[Edwin Török]

paths.ml contains various paths known to configure,
and currently is generated via a Makefile rule.
Simplify this and generate it through configure, similar to how
oxenstored.conf is generated from oxenstored.conf.in.

This will allow to reuse the generated file more easily with Dune.

No functional change.

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
---
 tools/configure                   | 4 +++-
 tools/configure.ac                | 2 ++
 tools/ocaml/libs/xs/Makefile      | 5 -----
 tools/ocaml/libs/xs/paths.ml.in   | 1 +
 tools/ocaml/xenstored/Makefile    | 5 -----
 tools/ocaml/xenstored/paths.ml.in | 4 ++++
 6 files changed, 10 insertions(+), 11 deletions(-)
 create mode 100644 tools/ocaml/libs/xs/paths.ml.in
 create mode 100644 tools/ocaml/xenstored/paths.ml.in

diff --git a/tools/configure b/tools/configure
index a052c186a5..41deb7fb96 100755
--- a/tools/configure
+++ b/tools/configure
@@ -2453,7 +2453,7 @@ ac_compiler_gnu=$ac_cv_c_compiler_gnu
 
 
 
-ac_config_files="$ac_config_files ../config/Tools.mk hotplug/FreeBSD/rc.d/xencommons hotplug/FreeBSD/rc.d/xendriverdomain hotplug/Linux/init.d/sysconfig.xencommons hotplug/Linux/init.d/sysconfig.xendomains hotplug/Linux/init.d/xen-watchdog hotplug/Linux/init.d/xencommons hotplug/Linux/init.d/xendomains hotplug/Linux/init.d/xendriverdomain hotplug/Linux/launch-xenstore hotplug/Linux/vif-setup hotplug/Linux/xen-hotplug-common.sh hotplug/Linux/xendomains hotplug/NetBSD/rc.d/xencommons hotplug/NetBSD/rc.d/xendriverdomain ocaml/xenstored/oxenstored.conf"
+ac_config_files="$ac_config_files ../config/Tools.mk hotplug/FreeBSD/rc.d/xencommons hotplug/FreeBSD/rc.d/xendriverdomain hotplug/Linux/init.d/sysconfig.xencommons hotplug/Linux/init.d/sysconfig.xendomains hotplug/Linux/init.d/xen-watchdog hotplug/Linux/init.d/xencommons hotplug/Linux/init.d/xendomains hotplug/Linux/init.d/xendriverdomain hotplug/Linux/launch-xenstore hotplug/Linux/vif-setup hotplug/Linux/xen-hotplug-common.sh hotplug/Linux/xendomains hotplug/NetBSD/rc.d/xencommons hotplug/NetBSD/rc.d/xendriverdomain ocaml/libs/xs/paths.ml ocaml/xenstored/paths.ml ocaml/xenstored/oxenstored.conf"
 
 ac_config_headers="$ac_config_headers config.h"
 
@@ -10935,6 +10935,8 @@ do
     "hotplug/Linux/xendomains") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/xendomains" ;;
     "hotplug/NetBSD/rc.d/xencommons") CONFIG_FILES="$CONFIG_FILES hotplug/NetBSD/rc.d/xencommons" ;;
     "hotplug/NetBSD/rc.d/xendriverdomain") CONFIG_FILES="$CONFIG_FILES hotplug/NetBSD/rc.d/xendriverdomain" ;;
+    "ocaml/libs/xs/paths.ml") CONFIG_FILES="$CONFIG_FILES ocaml/libs/xs/paths.ml" ;;
+    "ocaml/xenstored/paths.ml") CONFIG_FILES="$CONFIG_FILES ocaml/xenstored/paths.ml" ;;
     "ocaml/xenstored/oxenstored.conf") CONFIG_FILES="$CONFIG_FILES ocaml/xenstored/oxenstored.conf" ;;
     "config.h") CONFIG_HEADERS="$CONFIG_HEADERS config.h" ;;
     "hotplug/Linux/systemd/proc-xen.mount") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/systemd/proc-xen.mount" ;;
diff --git a/tools/configure.ac b/tools/configure.ac
index 1094d896fc..32cbe6bd3c 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -21,6 +21,8 @@ hotplug/Linux/xen-hotplug-common.sh
 hotplug/Linux/xendomains
 hotplug/NetBSD/rc.d/xencommons
 hotplug/NetBSD/rc.d/xendriverdomain
+ocaml/libs/xs/paths.ml
+ocaml/xenstored/paths.ml
 ocaml/xenstored/oxenstored.conf
 ])
 AC_CONFIG_HEADERS([config.h])
diff --git a/tools/ocaml/libs/xs/Makefile b/tools/ocaml/libs/xs/Makefile
index e934bbb550..e160e6a711 100644
--- a/tools/ocaml/libs/xs/Makefile
+++ b/tools/ocaml/libs/xs/Makefile
@@ -44,8 +44,3 @@ uninstall:
 	$(OCAMLFIND) remove -destdir $(OCAMLDESTDIR) xenstore
 
 include $(OCAML_TOPLEVEL)/Makefile.rules
-
-genpath-target = $(call buildmakevars2module,paths.ml)
-$(eval $(genpath-target))
-
-GENERATED_FILES += paths.ml
diff --git a/tools/ocaml/libs/xs/paths.ml.in b/tools/ocaml/libs/xs/paths.ml.in
new file mode 100644
index 0000000000..c067f8d012
--- /dev/null
+++ b/tools/ocaml/libs/xs/paths.ml.in
@@ -0,0 +1 @@
+let xen_run_stored = "@XEN_RUN_STORED@"
diff --git a/tools/ocaml/xenstored/Makefile b/tools/ocaml/xenstored/Makefile
index 0b5711b507..6f7333926e 100644
--- a/tools/ocaml/xenstored/Makefile
+++ b/tools/ocaml/xenstored/Makefile
@@ -93,8 +93,3 @@ uninstall:
 	rm -f $(DESTDIR)$(sbindir)/oxenstored
 
 include $(OCAML_TOPLEVEL)/Makefile.rules
-
-genpath-target = $(call buildmakevars2module,paths.ml)
-$(eval $(genpath-target))
-
-GENERATED_FILES += paths.ml
diff --git a/tools/ocaml/xenstored/paths.ml.in b/tools/ocaml/xenstored/paths.ml.in
new file mode 100644
index 0000000000..37949dc8f3
--- /dev/null
+++ b/tools/ocaml/xenstored/paths.ml.in
@@ -0,0 +1,4 @@
+let xen_log_dir = "@XEN_LOG_DIR@"
+let xen_config_dir = "@XEN_CONFIG_DIR@"
+let xen_run_dir = "@XEN_RUN_DIR@"
+let xen_run_stored = "@XEN_RUN_STORED@"
-- 
2.34.1

Re: [PATCH v1 2/7] tools/ocaml/*/Makefile: generate paths.ml from configure

[Christian Lindig]

[-- Attachment #1: Type: text/plain, Size: 525 bytes --]



On 29 Jul 2022, at 18:53, Edwin Török <edvin.torok@citrix.com<mailto:edvin.torok@citrix.com>> wrote:

paths.ml contains various paths known to configure,
and currently is generated via a Makefile rule.
Simplify this and generate it through configure, similar to how
oxenstored.conf is generated from oxenstored.conf.in.

This will allow to reuse the generated file more easily with Dune.

No functional change.

Acked-by: Christian Lindig <christian.lindig@citrix.com<mailto:christian.lindig@citrix.com>>


[-- Attachment #2: Type: text/html, Size: 6019 bytes --]

Re: [PATCH v1 2/7] tools/ocaml/*/Makefile: generate paths.ml from configure

[Andrew Cooper]

On 29/07/2022 18:53, Edwin Török wrote:
> paths.ml contains various paths known to configure,
> and currently is generated via a Makefile rule.
> Simplify this and generate it through configure, similar to how
> oxenstored.conf is generated from oxenstored.conf.in.
>
> This will allow to reuse the generated file more easily with Dune.
>
> No functional change.
>
> Signed-off-by: Edwin Török <edvin.torok@citrix.com>

With this change, buildmakevars2module has no users and should be dropped.

Can be fixed on commit.

~Andrew

[PATCH v1 3/7] tools/ocaml/*/dune: dune based build system

[Edwin Török]

Based on Christian Lindig's work.

Initially this will be used to build unit tests, and to make development
easier.

Dune supports proper incremental builds and editor integration with
merlin/LSP.

For now the Makefile based build system is retained too: this is not a
hard dependency on Dune.

Using version 2.1 of Dune build language here, because that is the one
available in Ubuntu Focal (part of the CI here).

No functional change.

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
---
 tools/.gitignore               |  7 +++++
 tools/dune                     |  5 ++++
 tools/dune-project             |  1 +
 tools/ocaml/dune-project       | 27 ++++++++++++++++++
 tools/ocaml/libs/eventchn/dune | 11 ++++++++
 tools/ocaml/libs/mmap/dune     |  9 ++++++
 tools/ocaml/libs/xb/dune       | 10 +++++++
 tools/ocaml/libs/xc/dune       | 16 +++++++++++
 tools/ocaml/libs/xs/dune       | 15 ++++++++++
 tools/ocaml/xenstored/dune     | 51 ++++++++++++++++++++++++++++++++++
 10 files changed, 152 insertions(+)
 create mode 100644 tools/.gitignore
 create mode 100644 tools/dune
 create mode 100644 tools/dune-project
 create mode 100644 tools/ocaml/dune-project
 create mode 100644 tools/ocaml/libs/eventchn/dune
 create mode 100644 tools/ocaml/libs/mmap/dune
 create mode 100644 tools/ocaml/libs/xb/dune
 create mode 100644 tools/ocaml/libs/xc/dune
 create mode 100644 tools/ocaml/libs/xs/dune
 create mode 100644 tools/ocaml/xenstored/dune

diff --git a/tools/.gitignore b/tools/.gitignore
new file mode 100644
index 0000000000..c211749a3b
--- /dev/null
+++ b/tools/.gitignore
@@ -0,0 +1,7 @@
+dune-workspace*
+_build/
+.merlin
+*.h.gch
+*.opam
+ocaml/*.install
+include/_xentoolcore_list.h
diff --git a/tools/dune b/tools/dune
new file mode 100644
index 0000000000..febbd078f0
--- /dev/null
+++ b/tools/dune
@@ -0,0 +1,5 @@
+; only look inside ocaml and include subdirectory, speeds up the build
+; since dune doesn't need to copy/hash/monitor all the other files
+(dirs ocaml)
+
+(data_only_dirs include libs)
diff --git a/tools/dune-project b/tools/dune-project
new file mode 100644
index 0000000000..cd8d4e3d86
--- /dev/null
+++ b/tools/dune-project
@@ -0,0 +1 @@
+(lang dune 2.1)
diff --git a/tools/ocaml/dune-project b/tools/ocaml/dune-project
new file mode 100644
index 0000000000..1dae7b0acb
--- /dev/null
+++ b/tools/ocaml/dune-project
@@ -0,0 +1,27 @@
+(lang dune 2.1)
+
+(name xen)
+
+(formatting (enabled_for dune))
+(generate_opam_files true)
+
+(maintainers christian.lindig@citrix.com)
+(license LGPL)
+
+(package
+ (name xen)
+ (synopsis "Xen interfaces")
+ (depends
+  base-unix
+  (dune (>= 2.1))
+ )
+)
+
+(package
+ (name xenstored)
+ (synopsis "In-memory key-value store for the Xen hypervisor")
+ (depends
+  base-unix
+  (dune (>= 2.1))
+ )
+)
diff --git a/tools/ocaml/libs/eventchn/dune b/tools/ocaml/libs/eventchn/dune
new file mode 100644
index 0000000000..4468f2e769
--- /dev/null
+++ b/tools/ocaml/libs/eventchn/dune
@@ -0,0 +1,11 @@
+(library
+ (foreign_stubs
+  (language c)
+  (names xeneventchn_stubs)
+  (extra_deps ../../../include/xen/xen.h ../../../libs/evtchn/libxenevtchn.so)
+  (include_dirs ../../../include))
+ (name xeneventchn)
+ (public_name xen.eventchn)
+ (libraries unix)
+ (no_dynlink)
+ (c_library_flags -lxenevtchn))
diff --git a/tools/ocaml/libs/mmap/dune b/tools/ocaml/libs/mmap/dune
new file mode 100644
index 0000000000..57a8ab5b9b
--- /dev/null
+++ b/tools/ocaml/libs/mmap/dune
@@ -0,0 +1,9 @@
+(library
+ (foreign_stubs
+  (language c)
+  (names xenmmap_stubs))
+ (name xenmmap)
+ (public_name xen.mmap)
+ (libraries unix)
+ (no_dynlink)
+ (install_c_headers mmap_stubs))
diff --git a/tools/ocaml/libs/xb/dune b/tools/ocaml/libs/xb/dune
new file mode 100644
index 0000000000..13a507ea87
--- /dev/null
+++ b/tools/ocaml/libs/xb/dune
@@ -0,0 +1,10 @@
+(library
+ (foreign_stubs
+  (language c)
+  (extra_deps ../../../include/xen/xen.h)
+  (include_dirs ../../../include)
+  (names xenbus_stubs xs_ring_stubs))
+ (name xenbus)
+ (public_name xen.bus)
+ (no_dynlink)
+ (libraries unix xenmmap))
diff --git a/tools/ocaml/libs/xc/dune b/tools/ocaml/libs/xc/dune
new file mode 100644
index 0000000000..6f9450cd27
--- /dev/null
+++ b/tools/ocaml/libs/xc/dune
@@ -0,0 +1,16 @@
+(rule
+ (with-stdout-to
+  xenctrl_abi_check.h
+  (run perl -w %{dep:abi-check} %{dep:xenctrl_stubs.c} %{dep:xenctrl.ml})))
+
+(library
+ (foreign_stubs
+  (language c)
+  (names xenctrl_stubs)
+  (extra_deps ../../../include/xen/xen.h ../../../libs/ctrl/libxenctrl.so)
+  (include_dirs ../../../include))
+ (name xenctrl)
+ (public_name xen.ctrl)
+ (libraries unix xenmmap)
+ (no_dynlink)
+ (c_library_flags -lxenctrl -lxenguest))
diff --git a/tools/ocaml/libs/xs/dune b/tools/ocaml/libs/xs/dune
new file mode 100644
index 0000000000..086259f51d
--- /dev/null
+++ b/tools/ocaml/libs/xs/dune
@@ -0,0 +1,15 @@
+; fallback mode: the files may have been generated by configure already
+
+(rule
+ (targets paths.ml)
+ (deps paths.ml.in)
+ (mode fallback)
+ (action
+  (run ../../../config.status --file=paths.ml)))
+
+(library
+ ; avoid conflict with mirage lib: name it differently
+ (name xenstore_xen)
+ (public_name xen.store)
+ (no_dynlink)
+ (libraries unix xenbus))
diff --git a/tools/ocaml/xenstored/dune b/tools/ocaml/xenstored/dune
new file mode 100644
index 0000000000..d71decebcf
--- /dev/null
+++ b/tools/ocaml/xenstored/dune
@@ -0,0 +1,51 @@
+; fallback mode: the files may have been generated by configure already
+; also for fallback mode either all files must be present or none
+; hence the 2 separate rules here
+
+(rule
+ (targets oxenstored.conf)
+ (deps oxenstored.conf.in)
+ (mode fallback)
+ (action
+  (run ../../config.status --file=oxenstored.conf)))
+
+(rule
+ (targets paths.ml)
+ (deps paths.ml.in)
+ (mode fallback)
+ (action
+  (run ../../config.status --file=paths.ml)))
+
+(executable
+ (modes native)
+ (name xenstored)
+ (modules
+  (:standard \ syslog systemd))
+ (flags
+  (:standard -w -52))
+ (libraries unix xen.bus xen.mmap xen.ctrl xen.eventchn xenstubs))
+
+(install
+ (package xenstored)
+ (section sbin)
+ (files
+  (xenstored.exe as oxenstored)))
+
+(install
+ (package xen)
+ (section etc)
+ (files oxenstored.conf))
+
+(library
+ (foreign_stubs
+  (language c)
+  (names syslog_stubs systemd_stubs select_stubs)
+  (extra_deps ../../dune-workspace)
+  (flags
+   (:standard -DHAVE_SYSTEMD)))
+ (modules syslog systemd)
+ (name xenstubs)
+ (wrapped false)
+ (libraries unix)
+ (no_dynlink)
+ (c_library_flags -lsystemd))
-- 
2.34.1

Re: [PATCH v1 3/7] tools/ocaml/*/dune: dune based build system

[Christian Lindig]

[-- Attachment #1: Type: text/plain, Size: 1709 bytes --]



On 29 Jul 2022, at 18:53, Edwin Török <edvin.torok@citrix.com<mailto:edvin.torok@citrix.com>> wrote:

Based on Christian Lindig's work.

Initially this will be used to build unit tests, and to make development
easier.

Dune supports proper incremental builds and editor integration with
merlin/LSP.

For now the Makefile based build system is retained too: this is not a
hard dependency on Dune.

Using version 2.1 of Dune build language here, because that is the one
available in Ubuntu Focal (part of the CI here).

No functional change.

Signed-off-by: Edwin Török <edvin.torok@citrix.com<mailto:edvin.torok@citrix.com>>
---
tools/.gitignore               |  7 +++++
tools/dune                     |  5 ++++
tools/dune-project             |  1 +
tools/ocaml/dune-project       | 27 ++++++++++++++++++
tools/ocaml/libs/eventchn/dune | 11 ++++++++
tools/ocaml/libs/mmap/dune     |  9 ++++++
tools/ocaml/libs/xb/dune       | 10 +++++++
tools/ocaml/libs/xc/dune       | 16 +++++++++++
tools/ocaml/libs/xs/dune       | 15 ++++++++++
tools/ocaml/xenstored/dune     | 51 ++++++++++++++++++++++++++++++++++
10 files changed, 152 insertions(+)
create mode 100644 tools/.gitignore
create mode 100644 tools/dune
create mode 100644 tools/dune-project
create mode 100644 tools/ocaml/dune-project
create mode 100644 tools/ocaml/libs/eventchn/dune
create mode 100644 tools/ocaml/libs/mmap/dune
create mode 100644 tools/ocaml/libs/xb/dune
create mode 100644 tools/ocaml/libs/xc/dune
create mode 100644 tools/ocaml/libs/xs/dune
create mode 100644 tools/ocaml/xenstored/dune

Acked-by: Christian Lindig <christian.lindig@citrix.com<mailto:christian.lindig@citrix.com>>



[-- Attachment #2: Type: text/html, Size: 29002 bytes --]

Re: [PATCH v1 3/7] tools/ocaml/*/dune: dune based build system

[Anthony PERARD]

On Fri, Jul 29, 2022 at 06:53:26PM +0100, Edwin Török wrote:
> Based on Christian Lindig's work.

Should we have is "Signed-off-by" tag then? Also he might be the author
of the patch, isn't it?

> Initially this will be used to build unit tests, and to make development
> easier.
>
> Dune supports proper incremental builds and editor integration with
> merlin/LSP.
> 
> For now the Makefile based build system is retained too: this is not a
> hard dependency on Dune.
> 
> Using version 2.1 of Dune build language here, because that is the one
> available in Ubuntu Focal (part of the CI here).
> 
> No functional change.
> 
> Signed-off-by: Edwin Török <edvin.torok@citrix.com>
> ---
>  create mode 100644 tools/dune
>  create mode 100644 tools/dune-project

Should this two new "dune*" files be added to MAINTAINERS in the OCAML
section?

> diff --git a/tools/.gitignore b/tools/.gitignore
> new file mode 100644
> index 0000000000..c211749a3b
> --- /dev/null
> +++ b/tools/.gitignore
> @@ -0,0 +1,7 @@
> +dune-workspace*

Is this file going to be in many subdirectory or just in tools/ ? You can
prepend a slash to tell git to ignore this file only in this directory.

> +_build/

Is this where dune is going to do out-of-tree build by default? Also, is
this only going to be in tools/_build/ ?


> +.merlin
> +*.h.gch
> +*.opam
> +ocaml/*.install
> +include/_xentoolcore_list.h

That last line doesn't seems related to dune, why is it in this patch?

Also, can you sort the lines in this .gitignore?

Thanks,

-- 
Anthony PERARD

Re: [PATCH v1 3/7] tools/ocaml/*/dune: dune based build system

[Christian Lindig]

[-- Attachment #1: Type: text/plain, Size: 550 bytes --]



On 3 Aug 2022, at 12:25, Anthony PERARD <anthony.perard@citrix.com<mailto:anthony.perard@citrix.com>> wrote:

On Fri, Jul 29, 2022 at 06:53:26PM +0100, Edwin Török wrote:
Based on Christian Lindig's work.

Should we have is "Signed-off-by" tag then? Also he might be the author
of the patch, isn't it?

This only refers to my initial experiments to set up dune for building OCaml xenstore that I had shared with Edvin. If at all, there are only traces of this left and so I see no need to acknowledge this is some formal way.

— C

[-- Attachment #2: Type: text/html, Size: 3624 bytes --]

[PATCH v1 4/7] tools/ocaml: Makefile to drive dune

[Edwin Török]

create a separate Makefile that can be used to drive dune.

Usage:
`make -f Makefile.dune`

There are some files that need to be created by the Makefile based
build system (such as all the C code in $(XEN_ROOT)/tools/libs),
and those need to exist before dune runs.

Although it'd be possible to automatically call the necessary makefile
rules from dune, it wouldn't work reliably:
* dune uses sandboxing by default (only files declared or known as
  dependencies are visible to individual build commands,
  symlinks/hardlinks are used by dune to implement this)
* the dune builds always run in a _build subdir, and calling the
  makefiles from there would get the wrong XEN_ROOT set
* running the make command in the source tree would work, but dune still
  wouldn't immediately see the build dependencies since they wouldn't
  have been copied/linked under _build

The approach here is to:
* use the Makefile to build C-only prerequisites (i.e. most of Xen)
* use Dune only to build the OCaml parts once the C prerequisites exist
* dune has dependencies declared on the C bits, so if they are missing
  you will get an error about a missing rule to create them instead of a
  cryptic compilation error
* dune is still optional - the old Makefile based buildsystem is still
  there for now
* use dune exclusively for new code going forward (e.g. OCaml test-suites)

The workspace file needs to be generated by make because this currently
cannot be generated by dune, and it doesn't support including external
files. But could be generated by configure?

LD_LIBRARY_PATH needs to be set, because even with -Wl,-rpath
executables wouldn't be able to run using the just-built libraries,
unless we'd also link all the transitive dependencies of libs.

No functional change.

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
---
 Makefile                          |  5 ++
 tools/ocaml/Makefile.dune         | 88 +++++++++++++++++++++++++++++++
 tools/ocaml/dune-workspace.dev.in |  2 +
 tools/ocaml/dune-workspace.in     | 18 +++++++
 4 files changed, 113 insertions(+)
 create mode 100644 tools/ocaml/Makefile.dune
 create mode 100644 tools/ocaml/dune-workspace.dev.in
 create mode 100644 tools/ocaml/dune-workspace.in

diff --git a/Makefile b/Makefile
index b93b22c752..ddb33c3555 100644
--- a/Makefile
+++ b/Makefile
@@ -68,6 +68,11 @@ build-tools-oxenstored: build-tools-public-headers
 	$(MAKE) -s -C tools/libs
 	$(MAKE) -C tools/ocaml build-tools-oxenstored
 
+.PHONY: build-tools-oxenstored-prepare
+build-tools-oxenstored-prepare: build-tools-public-headers
+	test -f tools/config.status || (cd tools && ./configure --with-xenstored=oxenstored)
+	$(MAKE) -C tools/libs V=
+
 .PHONY: build-stubdom
 build-stubdom: mini-os-dir build-tools-public-headers
 	$(MAKE) -C stubdom build
diff --git a/tools/ocaml/Makefile.dune b/tools/ocaml/Makefile.dune
new file mode 100644
index 0000000000..eca9cac0ca
--- /dev/null
+++ b/tools/ocaml/Makefile.dune
@@ -0,0 +1,88 @@
+XEN_ROOT = $(CURDIR)/../..
+all: dune-all-check
+
+# Dune by default uses all available CPUs. Make doesn't.
+# Query the available CPUs and use all available for any of the make rules we call out to.
+# -O is also needed with parallel make such that the build error and the build command causing
+#  the error are close together and not interspersed with other output
+NPROC=$(shell getconf _NPROCESSORS_ONLN)
+MAKEN=$(MAKE) -j$(NPROC) -O
+
+# We want to link and use the Xen libraries built locally
+# without installing them system-wide
+# (the system-wide one installed from packages will likely be too old and not match the locally
+# built one anyway).
+#
+# Set LIBRARY_PATH and LD_LIBRARY_PATH so that the linker
+# finds the proper libraries and the various dune commands
+# work (e.g. running tests, utop, etc.).
+#
+# The Makefile based buildsystem would use -Wl,-rpath-link= here,
+# but that only works during linking, not runtime.
+# There is a -Wl, -rpath= that can be used, but that only works
+# for libraries linked directly to the main executable:
+# the dependencies of those libraries won't get found on the rpath
+# (the rpath of the executable is apparently not used during that search).
+#
+# Use environment variables, because that way we don't make any permanent alternations (rpath)
+# to the executable, so once installed system-wide it won't refer to build paths anymore.
+#
+# Dune cannot be used to generate this file: the env-vars stanza doesn't support %{read:}, :include,
+# and dune-workspace doesn't support (include) stanzas.
+# So for now generate it from this Makefile
+# Cannot start with comment, so add auto-generated comment at the end
+LIB_DIRS=$(abspath $(wildcard ../libs/*/.))
+LIBRARY_PATH=$(subst $(eval) ,:,$(LIB_DIRS))
+../dune-workspace ../dune-workspace.dev: dune-workspace.in dune-workspace.dev.in Makefile.dune
+	@( sed -e "s|@LIBRARY_PATH@|$(LIBRARY_PATH)|" <$< \
+	&& echo "; DO NOT EDIT: autogenerated from ocaml/dune-workspace.in") >../dune-workspace
+	@cat ../dune-workspace dune-workspace.dev.in >../dune-workspace.dev
+
+# for location of various libs which moves between Xen versions
+include $(XEN_ROOT)/tools/Rules.mk
+
+XEN_DEPS=$(XEN_libxenctrl)/libxenctrl.so
+XEN_DEPS+=$(XEN_libxenevtchn)/libxenevtchn.so
+XEN_DEPS+=$(XEN_libxenguest)/libxenguest.so
+
+# Cannot be generated from dune
+# Tell the user how to generate them
+../include/xen/xen.h ../config.status $(XEN_DEPS):
+	echo "Missing C headers or libraries" >&2
+	echo "Run make -C $(XEN_ROOT) build-tools-oxenstored-prepare -j$$(nproc)" >&2
+	exit 1
+
+# dune would refuse to run if there are build artifacts in the source directory
+# if we detect anything then run make clean to ensure these are removed
+# don't always call 'make clean' because it takes ~1.6s
+.PHONY: dune-pre
+dune-pre: ../config.status | ../include/xen/xen.h ../dune-workspace $(XEN_DEPS)
+	$(MAKEN) clean -s
+
+# Convenience targets
+dune-syntax-check: dune-pre
+	dune build @check
+
+dune-all-check: dune-pre ../dune-workspace.dev
+	# Test build with multiple compiler versions
+	# requires opam switches for each to be already installed
+	dune build --workspace=../dune-workspace.dev @check @install @runtest
+
+check: dune-pre
+	dune runtest --no-buffer
+
+# approximatively equivalent to Dune 3.0 --release mode
+dune-oxenstored: dune-pre
+	dune build --root .. --ignore-promoted-rules --no-config \
+           --profile release --always-show-command-line \
+           --promote-install-files --default-target @install
+
+-include $(XEN_ROOT)/config/Paths.mk
+
+# skip doc, it'd install an extra LICENSE file that is already installed by other rules
+INSTALL_SECTIONS=bin,etc,lib,sbin
+dune-install: dune-oxenstored
+	dune install --destdir=$(DESTDIR) --prefix=$(prefix) --libdir=$(shell ocamlfind printconf destdir) --mandir=$(mandir) --etcdir=$(sysconfdir) --docdir=$(docdir) --sections=$(INSTALL_SECTIONS)
+
+dune-uninstall: dune-oxenstored
+	dune uninstall --destdir=$(DESTDIR) --prefix=$(prefix) --libdir=$(shell ocamlfind printconf destdir) --mandir=$(mandir) --etcdir=$(sysconfdir) --docdir=$(docdir)
diff --git a/tools/ocaml/dune-workspace.dev.in b/tools/ocaml/dune-workspace.dev.in
new file mode 100644
index 0000000000..2ca831a048
--- /dev/null
+++ b/tools/ocaml/dune-workspace.dev.in
@@ -0,0 +1,2 @@
+(context default)
+(context (opam (switch 4.02.3) (profile release)))
diff --git a/tools/ocaml/dune-workspace.in b/tools/ocaml/dune-workspace.in
new file mode 100644
index 0000000000..c963a6e599
--- /dev/null
+++ b/tools/ocaml/dune-workspace.in
@@ -0,0 +1,18 @@
+(lang dune 2.1)
+
+(env
+  ; we need to support older compilers so don't make deprecation warnings fatal
+ (dev
+  (flags (:standard -w -3))
+   (env-vars
+    (LD_LIBRARY_PATH @LIBRARY_PATH@)
+    (LIBRARY_PATH @LIBRARY_PATH@)
+   ))
+ (release
+  (env-vars
+   (OCAMLRUNPARAM b)
+    (LD_LIBRARY_PATH @LIBRARY_PATH@)
+    (LIBRARY_PATH @LIBRARY_PATH@)
+  )
+  (flags (:standard -strict-sequence -strict-formats -principal -w @18))
+  (ocamlopt_flags -nodynlink)))
-- 
2.34.1

Re: [PATCH v1 4/7] tools/ocaml: Makefile to drive dune

[Anthony PERARD]

On Fri, Jul 29, 2022 at 06:53:27PM +0100, Edwin Török wrote:
> create a separate Makefile that can be used to drive dune.
> 
> Usage:
> `make -f Makefile.dune`
> 
> There are some files that need to be created by the Makefile based
> build system (such as all the C code in $(XEN_ROOT)/tools/libs),
> and those need to exist before dune runs.
> 
> Although it'd be possible to automatically call the necessary makefile
> rules from dune, it wouldn't work reliably:
> * dune uses sandboxing by default (only files declared or known as
>   dependencies are visible to individual build commands,
>   symlinks/hardlinks are used by dune to implement this)
> * the dune builds always run in a _build subdir, and calling the
>   makefiles from there would get the wrong XEN_ROOT set
> * running the make command in the source tree would work, but dune still
>   wouldn't immediately see the build dependencies since they wouldn't
>   have been copied/linked under _build
> 
> The approach here is to:
> * use the Makefile to build C-only prerequisites (i.e. most of Xen)
> * use Dune only to build the OCaml parts once the C prerequisites exist
> * dune has dependencies declared on the C bits, so if they are missing
>   you will get an error about a missing rule to create them instead of a
>   cryptic compilation error
> * dune is still optional - the old Makefile based buildsystem is still
>   there for now
> * use dune exclusively for new code going forward (e.g. OCaml test-suites)
> 
> The workspace file needs to be generated by make because this currently
> cannot be generated by dune, and it doesn't support including external
> files. But could be generated by configure?

Potentially, but ./configure doesn't have access to the list of
xen libraries, so I'm not sure it would be a good idea.

> LD_LIBRARY_PATH needs to be set, because even with -Wl,-rpath
> executables wouldn't be able to run using the just-built libraries,
> unless we'd also link all the transitive dependencies of libs.
> 
> No functional change.
> 
> Signed-off-by: Edwin Török <edvin.torok@citrix.com>
> ---
>  Makefile                          |  5 ++
>  tools/ocaml/Makefile.dune         | 88 +++++++++++++++++++++++++++++++
>  tools/ocaml/dune-workspace.dev.in |  2 +
>  tools/ocaml/dune-workspace.in     | 18 +++++++
>  4 files changed, 113 insertions(+)
>  create mode 100644 tools/ocaml/Makefile.dune
>  create mode 100644 tools/ocaml/dune-workspace.dev.in
>  create mode 100644 tools/ocaml/dune-workspace.in

You've added dune-workspace* to .gitignore in the previous patch, should
the addition be done in this patch instead? (Also feel free to create
"tools/ocaml/.gitignore".

> diff --git a/Makefile b/Makefile
> index b93b22c752..ddb33c3555 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -68,6 +68,11 @@ build-tools-oxenstored: build-tools-public-headers
>  	$(MAKE) -s -C tools/libs
>  	$(MAKE) -C tools/ocaml build-tools-oxenstored
>  
> +.PHONY: build-tools-oxenstored-prepare
> +build-tools-oxenstored-prepare: build-tools-public-headers
> +	test -f tools/config.status || (cd tools && ./configure --with-xenstored=oxenstored)

No, do not run ./configure from the makefile. ./configure needs to be
run before running make.

> +	$(MAKE) -C tools/libs V=

No, do not start a build of the libraries from the root make file. If a
user were to run `make build-tools-oxenstored-prepare build-tools`, we
would end up with 2 make running `make -C tools/libs` concurrently which
isn't going to end well.

> diff --git a/tools/ocaml/Makefile.dune b/tools/ocaml/Makefile.dune
> new file mode 100644
> index 0000000000..eca9cac0ca
> --- /dev/null
> +++ b/tools/ocaml/Makefile.dune
> @@ -0,0 +1,88 @@
> +XEN_ROOT = $(CURDIR)/../..
> +all: dune-all-check
> +
> +# Dune by default uses all available CPUs. Make doesn't.
> +# Query the available CPUs and use all available for any of the make rules we call out to.
> +# -O is also needed with parallel make such that the build error and the build command causing
> +#  the error are close together and not interspersed with other output
> +NPROC=$(shell getconf _NPROCESSORS_ONLN)
> +MAKEN=$(MAKE) -j$(NPROC) -O

Please, don't change those options, I don't think these options belong
to a Makefile.

> +# We want to link and use the Xen libraries built locally
> +# without installing them system-wide
> +# (the system-wide one installed from packages will likely be too old and not match the locally
> +# built one anyway).
> +#
> +# Set LIBRARY_PATH and LD_LIBRARY_PATH so that the linker
> +# finds the proper libraries and the various dune commands
> +# work (e.g. running tests, utop, etc.).
> +#
> +# The Makefile based buildsystem would use -Wl,-rpath-link= here,
> +# but that only works during linking, not runtime.
> +# There is a -Wl, -rpath= that can be used, but that only works
> +# for libraries linked directly to the main executable:
> +# the dependencies of those libraries won't get found on the rpath
> +# (the rpath of the executable is apparently not used during that search).

That's why you do -Lpath -Wl,-rpath=path. Would the files generated in
tools/pkg-config/ would be useful for dune?

LD_LIBRARY_PATH is kind of expected to run binaries, but how is
LIBRARY_PATH used, and by which process?

> +# Use environment variables, because that way we don't make any permanent alternations (rpath)
> +# to the executable, so once installed system-wide it won't refer to build paths anymore.
> +#
> +# Dune cannot be used to generate this file: the env-vars stanza doesn't support %{read:}, :include,
> +# and dune-workspace doesn't support (include) stanzas.
> +# So for now generate it from this Makefile
> +# Cannot start with comment, so add auto-generated comment at the end
> +LIB_DIRS=$(abspath $(wildcard ../libs/*/.))

Do you need all those libs? Can't you instead list the library needed
or use the value listed in "tools/libs/uselibs.mk" ?

> +LIBRARY_PATH=$(subst $(eval) ,:,$(LIB_DIRS))
> +../dune-workspace ../dune-workspace.dev: dune-workspace.in dune-workspace.dev.in Makefile.dune
> +	@( sed -e "s|@LIBRARY_PATH@|$(LIBRARY_PATH)|" <$< \
> +	&& echo "; DO NOT EDIT: autogenerated from ocaml/dune-workspace.in") >../dune-workspace
> +	@cat ../dune-workspace dune-workspace.dev.in >../dune-workspace.dev
> +
> +# for location of various libs which moves between Xen versions
> +include $(XEN_ROOT)/tools/Rules.mk
> +
> +XEN_DEPS=$(XEN_libxenctrl)/libxenctrl.so
> +XEN_DEPS+=$(XEN_libxenevtchn)/libxenevtchn.so
> +XEN_DEPS+=$(XEN_libxenguest)/libxenguest.so
> +
> +# Cannot be generated from dune
> +# Tell the user how to generate them
> +../include/xen/xen.h ../config.status $(XEN_DEPS):
> +	echo "Missing C headers or libraries" >&2
> +	echo "Run make -C $(XEN_ROOT) build-tools-oxenstored-prepare -j$$(nproc)" >&2
> +	exit 1
> +
> +# dune would refuse to run if there are build artifacts in the source directory
> +# if we detect anything then run make clean to ensure these are removed
> +# don't always call 'make clean' because it takes ~1.6s
> +.PHONY: dune-pre
> +dune-pre: ../config.status | ../include/xen/xen.h ../dune-workspace $(XEN_DEPS)
> +	$(MAKEN) clean -s

I think it would be much better to tell the user to run clean themself,
like we do in the hypervisor tree when trying to do an out-of-tree
build. See rule "outputmakefile" in "xen/Makefile".

> +
> +# Convenience targets
> +dune-syntax-check: dune-pre
> +	dune build @check
> +
> +dune-all-check: dune-pre ../dune-workspace.dev
> +	# Test build with multiple compiler versions
> +	# requires opam switches for each to be already installed
> +	dune build --workspace=../dune-workspace.dev @check @install @runtest
> +
> +check: dune-pre
> +	dune runtest --no-buffer
> +
> +# approximatively equivalent to Dune 3.0 --release mode
> +dune-oxenstored: dune-pre
> +	dune build --root .. --ignore-promoted-rules --no-config \
> +           --profile release --always-show-command-line \
> +           --promote-install-files --default-target @install
> +
> +-include $(XEN_ROOT)/config/Paths.mk

I think make should fail if "Paths.mk" doesn't exist, could you remove
the dash ? (Also, at this point, "Paths.mk" should already exist because
Rules.mk checks that ./configure has run.)
)
> +
> +# skip doc, it'd install an extra LICENSE file that is already installed by other rules
> +INSTALL_SECTIONS=bin,etc,lib,sbin
> +dune-install: dune-oxenstored
> +	dune install --destdir=$(DESTDIR) --prefix=$(prefix) --libdir=$(shell ocamlfind printconf destdir) --mandir=$(mandir) --etcdir=$(sysconfdir) --docdir=$(docdir) --sections=$(INSTALL_SECTIONS)

Each option here could be on there own line, for clarity.

> +
> +dune-uninstall: dune-oxenstored
> +	dune uninstall --destdir=$(DESTDIR) --prefix=$(prefix) --libdir=$(shell ocamlfind printconf destdir) --mandir=$(mandir) --etcdir=$(sysconfdir) --docdir=$(docdir)
> diff --git a/tools/ocaml/dune-workspace.dev.in b/tools/ocaml/dune-workspace.dev.in
> new file mode 100644
> index 0000000000..2ca831a048
> --- /dev/null
> +++ b/tools/ocaml/dune-workspace.dev.in
> @@ -0,0 +1,2 @@
> +(context default)
> +(context (opam (switch 4.02.3) (profile release)))
> diff --git a/tools/ocaml/dune-workspace.in b/tools/ocaml/dune-workspace.in
> new file mode 100644
> index 0000000000..c963a6e599
> --- /dev/null
> +++ b/tools/ocaml/dune-workspace.in
> @@ -0,0 +1,18 @@
> +(lang dune 2.1)
> +
> +(env
> +  ; we need to support older compilers so don't make deprecation warnings fatal
> + (dev
> +  (flags (:standard -w -3))
> +   (env-vars
> +    (LD_LIBRARY_PATH @LIBRARY_PATH@)
> +    (LIBRARY_PATH @LIBRARY_PATH@)
> +   ))
> + (release
> +  (env-vars
> +   (OCAMLRUNPARAM b)
> +    (LD_LIBRARY_PATH @LIBRARY_PATH@)

Shouldn't this line (and the next) been aligned with the previous one?

> +    (LIBRARY_PATH @LIBRARY_PATH@)
> +  )
> +  (flags (:standard -strict-sequence -strict-formats -principal -w @18))
> +  (ocamlopt_flags -nodynlink)))

Thanks,

-- 
Anthony PERARD

Re: [PATCH v1 4/7] tools/ocaml: Makefile to drive dune

[Edwin Torok]

> On 3 Aug 2022, at 14:46, Anthony PERARD <anthony.perard@citrix.com> wrote:
> 
> On Fri, Jul 29, 2022 at 06:53:27PM +0100, Edwin Török wrote:
>> create a separate Makefile that can be used to drive dune.
>> 
>> Usage:
>> `make -f Makefile.dune`
>> 
>> There are some files that need to be created by the Makefile based
>> build system (such as all the C code in $(XEN_ROOT)/tools/libs),
>> and those need to exist before dune runs.
>> 
>> Although it'd be possible to automatically call the necessary makefile
>> rules from dune, it wouldn't work reliably:
>> * dune uses sandboxing by default (only files declared or known as
>>  dependencies are visible to individual build commands,
>>  symlinks/hardlinks are used by dune to implement this)
>> * the dune builds always run in a _build subdir, and calling the
>>  makefiles from there would get the wrong XEN_ROOT set
>> * running the make command in the source tree would work, but dune still
>>  wouldn't immediately see the build dependencies since they wouldn't
>>  have been copied/linked under _build
>> 
>> The approach here is to:
>> * use the Makefile to build C-only prerequisites (i.e. most of Xen)
>> * use Dune only to build the OCaml parts once the C prerequisites exist
>> * dune has dependencies declared on the C bits, so if they are missing
>>  you will get an error about a missing rule to create them instead of a
>>  cryptic compilation error
>> * dune is still optional - the old Makefile based buildsystem is still
>>  there for now
>> * use dune exclusively for new code going forward (e.g. OCaml test-suites)
>> 
>> The workspace file needs to be generated by make because this currently
>> cannot be generated by dune, and it doesn't support including external
>> files. But could be generated by configure?
> 
> Potentially, but ./configure doesn't have access to the list of
> xen libraries, so I'm not sure it would be a good idea.

ok I'll remove it from the commit message.

> 
>> LD_LIBRARY_PATH needs to be set, because even with -Wl,-rpath
>> executables wouldn't be able to run using the just-built libraries,
>> unless we'd also link all the transitive dependencies of libs.
>> 
>> No functional change.
>> 
>> Signed-off-by: Edwin Török <edvin.torok@citrix.com>
>> ---
>> Makefile                          |  5 ++
>> tools/ocaml/Makefile.dune         | 88 +++++++++++++++++++++++++++++++
>> tools/ocaml/dune-workspace.dev.in |  2 +
>> tools/ocaml/dune-workspace.in     | 18 +++++++
>> 4 files changed, 113 insertions(+)
>> create mode 100644 tools/ocaml/Makefile.dune
>> create mode 100644 tools/ocaml/dune-workspace.dev.in
>> create mode 100644 tools/ocaml/dune-workspace.in
> 
> You've added dune-workspace* to .gitignore in the previous patch, should
> the addition be done in this patch instead? (Also feel free to create
> "tools/ocaml/.gitignore".
> 
>> diff --git a/Makefile b/Makefile
>> index b93b22c752..ddb33c3555 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -68,6 +68,11 @@ build-tools-oxenstored: build-tools-public-headers
>> 	$(MAKE) -s -C tools/libs
>> 	$(MAKE) -C tools/ocaml build-tools-oxenstored
>> 
>> +.PHONY: build-tools-oxenstored-prepare
>> +build-tools-oxenstored-prepare: build-tools-public-headers
>> +	test -f tools/config.status || (cd tools && ./configure --with-xenstored=oxenstored)
> 
> No, do not run ./configure from the makefile. ./configure needs to be
> run before running make.


Perhaps I can add the necessary instructions to a README in tools/ocaml instead of this Makefile rule (which was here for documentation/convenience reasons).
The toplevel configure can fail due to various missing dependencies, but for OCaml just the configure in tools should be sufficient.

> 
>> +	$(MAKE) -C tools/libs V=
> 
> No, do not start a build of the libraries from the root make file. If a
> user were to run `make build-tools-oxenstored-prepare build-tools`, we
> would end up with 2 make running `make -C tools/libs` concurrently which
> isn't going to end well.


I'd like a single command to build everything needed related to oxenstored, without necessarily building the rest of Xen (which could either take a long time, or fail due to missing dependencies).
Ideally Xen wouldn't use recursive invocations of make, but just one single makefile that is aware of all source files (and you could then refer to objects/libraries in another directory as dependencies)
and what I'd like to do could be achieved by simply asking 'make' to build tools/ocaml/xenstored/oxenstored and let it figure out the minimal set of code that needs to be built for that.
However such a change would be quite invasive to the build system (and there probably was a reason to use recursive makefiles, they might have some advantage I'm not aware of).
Where do you recommend to put this rule instead, should it be in `tools/ocaml`? (although in that case it'd have to do a make invocation in `../libs` which isn't necessarily nicer)

Or should it be a shell script instead that invokes all the necessary make rules with the right flags, e.g. tools/ocaml/dev-build.sh?
 (and in that case there'd be no risk of the user running multiple make rules if the script itself takes no parameters).

> 
>> diff --git a/tools/ocaml/Makefile.dune b/tools/ocaml/Makefile.dune
>> new file mode 100644
>> index 0000000000..eca9cac0ca
>> --- /dev/null
>> +++ b/tools/ocaml/Makefile.dune
>> @@ -0,0 +1,88 @@
>> +XEN_ROOT = $(CURDIR)/../..
>> +all: dune-all-check
>> +
>> +# Dune by default uses all available CPUs. Make doesn't.
>> +# Query the available CPUs and use all available for any of the make rules we call out to.
>> +# -O is also needed with parallel make such that the build error and the build command causing
>> +#  the error are close together and not interspersed with other output
>> +NPROC=$(shell getconf _NPROCESSORS_ONLN)
>> +MAKEN=$(MAKE) -j$(NPROC) -O
> 
> Please, don't change those options, I don't think these options belong
> to a Makefile.


this Makefile is not (yet) linked to the toplevel makefiles, it is only used it you explicitly
'make -f Makefile.dune', in which case it saves some typing if you don't have to specify -j every time.
Would there be another way to achieve this? e.g. a dotfile in user home that make knows about?
(I think rpmbuild can be configured that way to use the correct -j flag, but I'm not aware whether Make can).
Perhaps the above dev-build.sh would be the solution here too (move the settings that don't belong into a makefile into a convenience shell script)

> 
>> +# We want to link and use the Xen libraries built locally
>> +# without installing them system-wide
>> +# (the system-wide one installed from packages will likely be too old and not match the locally
>> +# built one anyway).
>> +#
>> +# Set LIBRARY_PATH and LD_LIBRARY_PATH so that the linker
>> +# finds the proper libraries and the various dune commands
>> +# work (e.g. running tests, utop, etc.).
>> +#
>> +# The Makefile based buildsystem would use -Wl,-rpath-link= here,
>> +# but that only works during linking, not runtime.
>> +# There is a -Wl, -rpath= that can be used, but that only works
>> +# for libraries linked directly to the main executable:
>> +# the dependencies of those libraries won't get found on the rpath
>> +# (the rpath of the executable is apparently not used during that search).
> 
> That's why you do -Lpath -Wl,-rpath=path. Would the files generated in
> tools/pkg-config/ would be useful for dune?
> 
> LD_LIBRARY_PATH is kind of expected to run binaries, but how is
> LIBRARY_PATH used, and by which process?

I think LIBRARY_PATH is used by gcc/ld to find the just built libraries. I can try without them, but ISTR linking failing.
I could use the rpath flags, but then my binary would end up with rpaths inside, which isn't necessarily what I want
(although I think that is what happens currently). The overriden rpath/library_path/ld_library_path is only needed on the development machine,
not when deployed onto a box with the rest of the libraries installed into the correct places.

I think distributions would typically remove all the rpath handling code anyway, so the only user of rpaths left would be developers,
where setting LD_LIBRARY_PATH/LIBRARY_PATH is less intrusive than modifying all the build rules to add the rpaths.
I can try to see whether there is a non-intrusive way of adding rpaths, perhaps including a certain file wherever linker flags are specified,
which could be initially empty, but could contain rpaths when needed (or other compiler/linker flags).
Then at least rpath handling would be done in only one place (and only one place to immediately undo in the patchqueue).

> 
>> +# Use environment variables, because that way we don't make any permanent alternations (rpath)
>> +# to the executable, so once installed system-wide it won't refer to build paths anymore.
>> +#
>> +# Dune cannot be used to generate this file: the env-vars stanza doesn't support %{read:}, :include,
>> +# and dune-workspace doesn't support (include) stanzas.
>> +# So for now generate it from this Makefile
>> +# Cannot start with comment, so add auto-generated comment at the end
>> +LIB_DIRS=$(abspath $(wildcard ../libs/*/.))
> 
> Do you need all those libs? Can't you instead list the library needed
> or use the value listed in "tools/libs/uselibs.mk" ?

Indeed, I think some Xen source paths changed since this patch was originally written, an explicit list is probably a better choice now,
since there are a lot of libs there that are not necessarily needed (e.g. xenlight)

> 
>> +LIBRARY_PATH=$(subst $(eval) ,:,$(LIB_DIRS))
>> +../dune-workspace ../dune-workspace.dev: dune-workspace.in dune-workspace.dev.in Makefile.dune
>> +	@( sed -e "s|@LIBRARY_PATH@|$(LIBRARY_PATH)|" <$< \
>> +	&& echo "; DO NOT EDIT: autogenerated from ocaml/dune-workspace.in") >../dune-workspace
>> +	@cat ../dune-workspace dune-workspace.dev.in >../dune-workspace.dev
>> +
>> +# for location of various libs which moves between Xen versions
>> +include $(XEN_ROOT)/tools/Rules.mk
>> +
>> +XEN_DEPS=$(XEN_libxenctrl)/libxenctrl.so
>> +XEN_DEPS+=$(XEN_libxenevtchn)/libxenevtchn.so
>> +XEN_DEPS+=$(XEN_libxenguest)/libxenguest.so
>> +
>> +# Cannot be generated from dune
>> +# Tell the user how to generate them
>> +../include/xen/xen.h ../config.status $(XEN_DEPS):
>> +	echo "Missing C headers or libraries" >&2
>> +	echo "Run make -C $(XEN_ROOT) build-tools-oxenstored-prepare -j$$(nproc)" >&2
>> +	exit 1
>> +
>> +# dune would refuse to run if there are build artifacts in the source directory
>> +# if we detect anything then run make clean to ensure these are removed
>> +# don't always call 'make clean' because it takes ~1.6s
>> +.PHONY: dune-pre
>> +dune-pre: ../config.status | ../include/xen/xen.h ../dune-workspace $(XEN_DEPS)
>> +	$(MAKEN) clean -s
> 
> I think it would be much better to tell the user to run clean themself,
> like we do in the hypervisor tree when trying to do an out-of-tree
> build. See rule "outputmakefile" in "xen/Makefile".


I could attempt to detect an unclean tree and abort the build instead with a message saying to run 'make clean'.
However detecting an unclean tree isn't necessarily trivial (although I think dune itself would detect and abort the build, so perhaps I can reuse that,
I'll have to do some experiments).
Does Xen support out-of-tree builds btw? That might be another option in maintaining a clean source tree without build artifacts.

> 
>> +
>> +# Convenience targets
>> +dune-syntax-check: dune-pre
>> +	dune build @check
>> +
>> +dune-all-check: dune-pre ../dune-workspace.dev
>> +	# Test build with multiple compiler versions
>> +	# requires opam switches for each to be already installed
>> +	dune build --workspace=../dune-workspace.dev @check @install @runtest
>> +
>> +check: dune-pre
>> +	dune runtest --no-buffer
>> +
>> +# approximatively equivalent to Dune 3.0 --release mode
>> +dune-oxenstored: dune-pre
>> +	dune build --root .. --ignore-promoted-rules --no-config \
>> +           --profile release --always-show-command-line \
>> +           --promote-install-files --default-target @install
>> +
>> +-include $(XEN_ROOT)/config/Paths.mk
> 
> I think make should fail if "Paths.mk" doesn't exist, could you remove
> the dash ? (Also, at this point, "Paths.mk" should already exist because
> Rules.mk checks that ./configure has run.)

Ok 

> )
>> +
>> +# skip doc, it'd install an extra LICENSE file that is already installed by other rules
>> +INSTALL_SECTIONS=bin,etc,lib,sbin
>> +dune-install: dune-oxenstored
>> +	dune install --destdir=$(DESTDIR) --prefix=$(prefix) --libdir=$(shell ocamlfind printconf destdir) --mandir=$(mandir) --etcdir=$(sysconfdir) --docdir=$(docdir) --sections=$(INSTALL_SECTIONS)
> 
> Each option here could be on there own line, for clarity.

Ok

> 
>> +
>> +dune-uninstall: dune-oxenstored
>> +	dune uninstall --destdir=$(DESTDIR) --prefix=$(prefix) --libdir=$(shell ocamlfind printconf destdir) --mandir=$(mandir) --etcdir=$(sysconfdir) --docdir=$(docdir)
>> diff --git a/tools/ocaml/dune-workspace.dev.in b/tools/ocaml/dune-workspace.dev.in
>> new file mode 100644
>> index 0000000000..2ca831a048
>> --- /dev/null
>> +++ b/tools/ocaml/dune-workspace.dev.in
>> @@ -0,0 +1,2 @@
>> +(context default)
>> +(context (opam (switch 4.02.3) (profile release)))
>> diff --git a/tools/ocaml/dune-workspace.in b/tools/ocaml/dune-workspace.in
>> new file mode 100644
>> index 0000000000..c963a6e599
>> --- /dev/null
>> +++ b/tools/ocaml/dune-workspace.in
>> @@ -0,0 +1,18 @@
>> +(lang dune 2.1)
>> +
>> +(env
>> +  ; we need to support older compilers so don't make deprecation warnings fatal
>> + (dev
>> +  (flags (:standard -w -3))
>> +   (env-vars
>> +    (LD_LIBRARY_PATH @LIBRARY_PATH@)
>> +    (LIBRARY_PATH @LIBRARY_PATH@)
>> +   ))
>> + (release
>> +  (env-vars
>> +   (OCAMLRUNPARAM b)
>> +    (LD_LIBRARY_PATH @LIBRARY_PATH@)
> 
> Shouldn't this line (and the next) been aligned with the previous one?

Yes

> 
>> +    (LIBRARY_PATH @LIBRARY_PATH@)
>> +  )
>> +  (flags (:standard -strict-sequence -strict-formats -principal -w @18))
>> +  (ocamlopt_flags -nodynlink)))


Thanks for the feedback, I'll think about how to best address some of these points (see above for some initial thoughts) and send out a V2 when ready.

Best regards,
--Edwin

Re: [PATCH v1 4/7] tools/ocaml: Makefile to drive dune

[Anthony PERARD]

On Wed, Aug 03, 2022 at 03:37:19PM +0000, Edwin Torok wrote:
> > On 3 Aug 2022, at 14:46, Anthony PERARD <anthony.perard@citrix.com> wrote:
> > On Fri, Jul 29, 2022 at 06:53:27PM +0100, Edwin Török wrote:
> >> +.PHONY: build-tools-oxenstored-prepare
> >> +build-tools-oxenstored-prepare: build-tools-public-headers
> >> +	test -f tools/config.status || (cd tools && ./configure --with-xenstored=oxenstored)
> > 
> > No, do not run ./configure from the makefile. ./configure needs to be
> > run before running make.
> 
> 
> Perhaps I can add the necessary instructions to a README in tools/ocaml instead of this Makefile rule (which was here for documentation/convenience reasons).

That would be fine.

> The toplevel configure can fail due to various missing dependencies, but for OCaml just the configure in tools should be sufficient.

:-), it doesn't looks like the others ./configure have much dependencies
that aren't also needed in tools, so I don't think it make sense to
avoid the toplevel ./configure at this time. We don't really test
running ./configure in only a subsystem so it's probably best to avoid
documenting it. You can always run `./configure --disable-{xen,docs}` to
run the minimum amount of ./configure.

> > 
> >> +	$(MAKE) -C tools/libs V=
> > 
> > No, do not start a build of the libraries from the root make file. If a
> > user were to run `make build-tools-oxenstored-prepare build-tools`, we
> > would end up with 2 make running `make -C tools/libs` concurrently which
> > isn't going to end well.
> 
> 
> I'd like a single command to build everything needed related to oxenstored, without necessarily building the rest of Xen (which could either take a long time, or fail due to missing dependencies).

Easy:
    ./configure && make -C tools/include && make -C tools/libs
Hopefully, this would be enough, will keep working for a while. But it
might break. (I would try to keep that command above working but who
knows if change would be needed)

> Ideally Xen wouldn't use recursive invocations of make, but just one single makefile that is aware of all source files (and you could then refer to objects/libraries in another directory as dependencies)
> and what I'd like to do could be achieved by simply asking 'make' to build tools/ocaml/xenstored/oxenstored and let it figure out the minimal set of code that needs to be built for that.
> However such a change would be quite invasive to the build system (and there probably was a reason to use recursive makefiles, they might have some advantage I'm not aware of).

Non-recursive makefiles, I want that as well!! :-)
I'm working on it:
    [XEN PATCH v3 00/25] Toolstack build system improvement, toward non-recursive makefiles
    https://lore.kernel.org/all/20220624160422.53457-1-anthony.perard@citrix.com/
But that's going to take a while. There's till a lot of patches that I
haven't posted yet.

> Where do you recommend to put this rule instead, should it be in `tools/ocaml`? (although in that case it'd have to do a make invocation in `../libs` which isn't necessarily nicer)
> 
> Or should it be a shell script instead that invokes all the necessary make rules with the right flags, e.g. tools/ocaml/dev-build.sh?
>  (and in that case there'd be no risk of the user running multiple make rules if the script itself takes no parameters).

I'm not sure, these are just kind of "optimisation" to workaround our
build system. It could easily apply to many subdir in tools/, but
there's no documentation for it. One could run make in most subdir after
just running `./configure && make -C tools/include && make -C
tools/libs`, but I don't think we should document it. As documenting it
makes it harder to make changes if needed.

> >> diff --git a/tools/ocaml/Makefile.dune b/tools/ocaml/Makefile.dune
> >> new file mode 100644
> >> index 0000000000..eca9cac0ca
> >> --- /dev/null
> >> +++ b/tools/ocaml/Makefile.dune
> >> @@ -0,0 +1,88 @@
> >> +XEN_ROOT = $(CURDIR)/../..
> >> +all: dune-all-check
> >> +
> >> +# Dune by default uses all available CPUs. Make doesn't.
> >> +# Query the available CPUs and use all available for any of the make rules we call out to.
> >> +# -O is also needed with parallel make such that the build error and the build command causing
> >> +#  the error are close together and not interspersed with other output
> >> +NPROC=$(shell getconf _NPROCESSORS_ONLN)
> >> +MAKEN=$(MAKE) -j$(NPROC) -O
> > 
> > Please, don't change those options, I don't think these options belong
> > to a Makefile.
> 
> 
> this Makefile is not (yet) linked to the toplevel makefiles, it is only used it you explicitly
> 'make -f Makefile.dune', in which case it saves some typing if you don't have to specify -j every time.

Use the "alias" built-in from your shell, if you want to have -j$(nproc)
added to you `make` commands.

> Would there be another way to achieve this? e.g. a dotfile in user home that make knows about?

There's .bashrc or .zshrc or ..., make doesn't need to know. ;-)

> (I think rpmbuild can be configured that way to use the correct -j flag, but I'm not aware whether Make can).
> Perhaps the above dev-build.sh would be the solution here too (move the settings that don't belong into a makefile into a convenience shell script)
> 
> > 
> >> +# We want to link and use the Xen libraries built locally
> >> +# without installing them system-wide
> >> +# (the system-wide one installed from packages will likely be too old and not match the locally
> >> +# built one anyway).
> >> +#
> >> +# Set LIBRARY_PATH and LD_LIBRARY_PATH so that the linker
> >> +# finds the proper libraries and the various dune commands
> >> +# work (e.g. running tests, utop, etc.).
> >> +#
> >> +# The Makefile based buildsystem would use -Wl,-rpath-link= here,
> >> +# but that only works during linking, not runtime.
> >> +# There is a -Wl, -rpath= that can be used, but that only works
> >> +# for libraries linked directly to the main executable:
> >> +# the dependencies of those libraries won't get found on the rpath
> >> +# (the rpath of the executable is apparently not used during that search).
> > 
> > That's why you do -Lpath -Wl,-rpath=path. Would the files generated in
> > tools/pkg-config/ would be useful for dune?
> > 
> > LD_LIBRARY_PATH is kind of expected to run binaries, but how is
> > LIBRARY_PATH used, and by which process?
> 
> I think LIBRARY_PATH is used by gcc/ld to find the just built libraries. I can try without them, but ISTR linking failing.
> I could use the rpath flags, but then my binary would end up with rpaths inside, which isn't necessarily what I want
> (although I think that is what happens currently). The overriden rpath/library_path/ld_library_path is only needed on the development machine,
> not when deployed onto a box with the rest of the libraries installed into the correct places.
> 
> I think distributions would typically remove all the rpath handling code anyway, so the only user of rpaths left would be developers,
> where setting LD_LIBRARY_PATH/LIBRARY_PATH is less intrusive than modifying all the build rules to add the rpaths.
> I can try to see whether there is a non-intrusive way of adding rpaths, perhaps including a certain file wherever linker flags are specified,
> which could be initially empty, but could contain rpaths when needed (or other compiler/linker flags).
> Then at least rpath handling would be done in only one place (and only one place to immediately undo in the patchqueue).

Use of $LD_LIBRARY_PATH is fine, it's already used in several places. I
guess "-Wl,-rpath" could be used instead but is probably best to avoid as
not normally needed.

Now about link time, normally we seem to have the full path to the
library we want to link to, or we provide '-L' via pkg-config file.
Then, when a library is links against another one, and the linker wants
to know where this library is, we use "-rpath-link" and I might be
wrong but this probably doesn't add anything in the library.

It seems to me that $LIBRARY_PATH might be useful for external project,
we would want to links against the libraries that aren't yet installed
on the system. But event that isn't needed as we generates
"tools/pkg-config/" which have the needed flags. We use that for
building qemu for example, and I think qemu's configure can make use of
that.

> >> +# Use environment variables, because that way we don't make any permanent alternations (rpath)
> >> +# to the executable, so once installed system-wide it won't refer to build paths anymore.
> >> +#
> >> +# Dune cannot be used to generate this file: the env-vars stanza doesn't support %{read:}, :include,
> >> +# and dune-workspace doesn't support (include) stanzas.
> >> +# So for now generate it from this Makefile
> >> +# Cannot start with comment, so add auto-generated comment at the end
> >> +LIB_DIRS=$(abspath $(wildcard ../libs/*/.))
> > 
> > Do you need all those libs? Can't you instead list the library needed
> > or use the value listed in "tools/libs/uselibs.mk" ?
> 
> Indeed, I think some Xen source paths changed since this patch was originally written, an explicit list is probably a better choice now,
> since there are a lot of libs there that are not necessarily needed (e.g. xenlight)

I'd mostly like to avoid $(wildcard ) if possible.

> >> +LIBRARY_PATH=$(subst $(eval) ,:,$(LIB_DIRS))
> >> +../dune-workspace ../dune-workspace.dev: dune-workspace.in dune-workspace.dev.in Makefile.dune
> >> +	@( sed -e "s|@LIBRARY_PATH@|$(LIBRARY_PATH)|" <$< \
> >> +	&& echo "; DO NOT EDIT: autogenerated from ocaml/dune-workspace.in") >../dune-workspace
> >> +	@cat ../dune-workspace dune-workspace.dev.in >../dune-workspace.dev
> >> +
> >> +# for location of various libs which moves between Xen versions
> >> +include $(XEN_ROOT)/tools/Rules.mk
> >> +
> >> +XEN_DEPS=$(XEN_libxenctrl)/libxenctrl.so
> >> +XEN_DEPS+=$(XEN_libxenevtchn)/libxenevtchn.so
> >> +XEN_DEPS+=$(XEN_libxenguest)/libxenguest.so
> >> +
> >> +# Cannot be generated from dune
> >> +# Tell the user how to generate them
> >> +../include/xen/xen.h ../config.status $(XEN_DEPS):
> >> +	echo "Missing C headers or libraries" >&2
> >> +	echo "Run make -C $(XEN_ROOT) build-tools-oxenstored-prepare -j$$(nproc)" >&2
> >> +	exit 1
> >> +
> >> +# dune would refuse to run if there are build artifacts in the source directory
> >> +# if we detect anything then run make clean to ensure these are removed
> >> +# don't always call 'make clean' because it takes ~1.6s
> >> +.PHONY: dune-pre
> >> +dune-pre: ../config.status | ../include/xen/xen.h ../dune-workspace $(XEN_DEPS)
> >> +	$(MAKEN) clean -s
> > 
> > I think it would be much better to tell the user to run clean themself,
> > like we do in the hypervisor tree when trying to do an out-of-tree
> > build. See rule "outputmakefile" in "xen/Makefile".
> 
> 
> I could attempt to detect an unclean tree and abort the build instead with a message saying to run 'make clean'.

The comment already speak detecting unclean source tree, but I guess it
probably still run `make clean` even if clean.

> However detecting an unclean tree isn't necessarily trivial (although I think dune itself would detect and abort the build, so perhaps I can reuse that,
> I'll have to do some experiments).
> Does Xen support out-of-tree builds btw? That might be another option in maintaining a clean source tree without build artifacts.

The toolstack build system doesn't support out-of-tree build. But my
work on non-recursive makefile would potentially allow that.

Having the dune stuff not been run when running for example `make
build-tools` is probably going to make this kind of things awkward at
first. That is both dune and make buids could be use in parallel (not
necessary at the same time) if the developer doesn't pay attention. I
guess later if could have something like `./configure
--enable-ocaml-dune`, there will be less risk of having the make build
ocaml stuff instead of dune as intended by a developer. Also, this could
allow to test dune build in our CI.

Cheers,

-- 
Anthony PERARD

[PATCH v1 5/7] tools/ocaml: fix compiler warnings

[Edwin Török]

Fix compiler warning about:
* unused value
* ambiguous documentation comment
* non-principal type inference (compiler version dependent)

No functional change.

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
---
 tools/ocaml/xenstored/connection.ml | 2 +-
 tools/ocaml/xenstored/process.ml    | 5 +++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/ocaml/xenstored/connection.ml b/tools/ocaml/xenstored/connection.ml
index 65f99ea6f2..a94d47cdc2 100644
--- a/tools/ocaml/xenstored/connection.ml
+++ b/tools/ocaml/xenstored/connection.ml
@@ -313,7 +313,7 @@ let is_bad con = match con.dom with None -> false | Some dom -> Domain.is_bad_do
 let has_extra_connection_data con =
 	let has_in = has_input con || has_partial_input con in
 	let has_out = has_output con in
-	let has_socket = con.dom = None in
+	let _has_socket = con.dom = None in
 	let has_nondefault_perms = make_perm con.dom <> con.perm in
 	has_in || has_out
 	(* TODO: what about SIGTERM, should use systemd to store FDS
diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml
index 27790d4a5c..86eed02413 100644
--- a/tools/ocaml/xenstored/process.ml
+++ b/tools/ocaml/xenstored/process.ml
@@ -59,7 +59,7 @@ let split_one_path data con =
 
 let process_watch t cons =
 	let oldroot = t.Transaction.oldroot in
-	let newroot = Store.get_root t.store in
+	let newroot = Store.get_root t.Transaction.store in
 	let ops = Transaction.get_paths t |> List.rev in
 	let do_op_watch op cons =
 		let recurse, oldroot, root = match (fst op) with
@@ -491,7 +491,7 @@ let transaction_replay c t doms cons =
 			ignore @@ Connection.end_transaction c tid None
 		)
 
-let do_watch con t _domains cons data =
+let do_watch con _t _domains cons data =
 	let (node, token) =
 		match (split None '\000' data) with
 		| [node; token; ""]   -> node, token
@@ -651,6 +651,7 @@ let maybe_ignore_transaction = function
 
 
 let () = Printexc.record_backtrace true
+
 (**
  * Nothrow guarantee.
  *)
-- 
2.34.1

Re: [PATCH v1 5/7] tools/ocaml: fix compiler warnings

[Christian Lindig]

[-- Attachment #1: Type: text/plain, Size: 374 bytes --]



On 29 Jul 2022, at 18:53, Edwin Török <edvin.torok@citrix.com<mailto:edvin.torok@citrix.com>> wrote:

Fix compiler warning about:
* unused value
* ambiguous documentation comment
* non-principal type inference (compiler version dependent)

No functional change.

Acked-by: Christian Lindig <christian.lindig@citrix.com<mailto:christian.lindig@citrix.com>>


[-- Attachment #2: Type: text/html, Size: 4820 bytes --]

Re: [PATCH v1 5/7] tools/ocaml: fix compiler warnings

[Andrew Cooper]

On 29/07/2022 18:53, Edwin Török wrote:
> diff --git a/tools/ocaml/xenstored/connection.ml b/tools/ocaml/xenstored/connection.ml
> index 65f99ea6f2..a94d47cdc2 100644
> --- a/tools/ocaml/xenstored/connection.ml
> +++ b/tools/ocaml/xenstored/connection.ml
> @@ -313,7 +313,7 @@ let is_bad con = match con.dom with None -> false | Some dom -> Domain.is_bad_do
>  let has_extra_connection_data con =
>  	let has_in = has_input con || has_partial_input con in
>  	let has_out = has_output con in
> -	let has_socket = con.dom = None in
> +	let _has_socket = con.dom = None in

There are no side effects here.  Can't the line simply be deleted?

~Andrew

Re: [PATCH v1 5/7] tools/ocaml: fix compiler warnings

[Christian Lindig]

[-- Attachment #1: Type: text/plain, Size: 359 bytes --]



On 3 Aug 2022, at 11:39, Andrew Cooper <Andrew.Cooper3@citrix.com<mailto:Andrew.Cooper3@citrix.com>> wrote:

There are no side effects here.  Can't the line simply be deleted?

Yes. The compiler tells us about unused bindings like these and this is the easy way to acknowledge this without removing the code but it could be removed as well.

— C

[-- Attachment #2: Type: text/html, Size: 1582 bytes --]

[PATCH v1 6/7] tools/ocaml/libs/xb: hide type of Xb.t

[Edwin Török]

The only user of 'xb' that I can find is in-tree oxenstored.
Other code (e.g. xenopsd) would use the mirage 'xenstore' implementation
instead, so changing the API here shouldn't require anyone to update
their code.

Hiding the type will make it easier to change the implementation
in the future without breaking code that relies on it.

No functional change.

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
---
 tools/ocaml/libs/xb/xb.ml           | 3 +++
 tools/ocaml/libs/xb/xb.mli          | 9 ++-------
 tools/ocaml/xenstored/connection.ml | 8 ++------
 3 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/tools/ocaml/libs/xb/xb.ml b/tools/ocaml/libs/xb/xb.ml
index 104d319d77..8404ddd8a6 100644
--- a/tools/ocaml/libs/xb/xb.ml
+++ b/tools/ocaml/libs/xb/xb.ml
@@ -196,6 +196,9 @@ let peek_output con = Queue.peek con.pkt_out
 let input_len con = Queue.length con.pkt_in
 let has_in_packet con = Queue.length con.pkt_in > 0
 let get_in_packet con = Queue.pop con.pkt_in
+let has_partial_input con = match con.partial_in with
+	| HaveHdr _ -> true
+	| NoHdr (n, _) -> n < Partial.header_size ()
 let has_more_input con =
 	match con.backend with
 	| Fd _         -> false
diff --git a/tools/ocaml/libs/xb/xb.mli b/tools/ocaml/libs/xb/xb.mli
index 3a00da6cdd..794e35bb34 100644
--- a/tools/ocaml/libs/xb/xb.mli
+++ b/tools/ocaml/libs/xb/xb.mli
@@ -66,13 +66,7 @@ type backend_mmap = {
 type backend_fd = { fd : Unix.file_descr; }
 type backend = Fd of backend_fd | Xenmmap of backend_mmap
 type partial_buf = HaveHdr of Partial.pkt | NoHdr of int * bytes
-type t = {
-  backend : backend;
-  pkt_in : Packet.t Queue.t;
-  pkt_out : Packet.t Queue.t;
-  mutable partial_in : partial_buf;
-  mutable partial_out : string;
-}
+type t
 val init_partial_in : unit -> partial_buf
 val reconnect : t -> unit
 val queue : t -> Packet.t -> unit
@@ -97,6 +91,7 @@ val has_output : t -> bool
 val peek_output : t -> Packet.t
 val input_len : t -> int
 val has_in_packet : t -> bool
+val has_partial_input : t -> bool
 val get_in_packet : t -> Packet.t
 val has_more_input : t -> bool
 val is_selectable : t -> bool
diff --git a/tools/ocaml/xenstored/connection.ml b/tools/ocaml/xenstored/connection.ml
index a94d47cdc2..0ce54cd7f9 100644
--- a/tools/ocaml/xenstored/connection.ml
+++ b/tools/ocaml/xenstored/connection.ml
@@ -125,9 +125,7 @@ let get_perm con =
 let set_target con target_domid =
 	con.perm <- Perms.Connection.set_target (get_perm con) ~perms:[Perms.READ; Perms.WRITE] target_domid
 
-let is_backend_mmap con = match con.xb.Xenbus.Xb.backend with
-	| Xenbus.Xb.Xenmmap _ -> true
-	| _ -> false
+let is_backend_mmap con = Xenbus.Xb.is_mmap con.xb
 
 let send_reply con tid rid ty data =
 	if (String.length data) > xenstore_payload_max && (is_backend_mmap con) then
@@ -280,9 +278,7 @@ let get_transaction con tid =
 
 let do_input con = Xenbus.Xb.input con.xb
 let has_input con = Xenbus.Xb.has_in_packet con.xb
-let has_partial_input con = match con.xb.Xenbus.Xb.partial_in with
-	| HaveHdr _ -> true
-	| NoHdr (n, _) -> n < Xenbus.Partial.header_size ()
+let has_partial_input con = Xenbus.Xb.has_partial_input con.xb
 let pop_in con = Xenbus.Xb.get_in_packet con.xb
 let has_more_input con = Xenbus.Xb.has_more_input con.xb
 
-- 
2.34.1

[PATCH v1 7/7] tools/ocaml/libs/eventchn: do not leak event channels and OCaml 5.0 compat

[Edwin Török]

Add a finalizer on the event channel value, so that it calls
`xenevtchn_close` when the value would be GCed.

In practice oxenstored seems to be the only user of this,
and it creates a single global event channel only,
but freeing this could still be useful when run with OCAMLRUNPARAM=c

The code was previously casting a C pointer to an OCaml value,
which should be avoided: OCaml 5.0 won't support it.
(all "naked" C pointers must be wrapped inside an OCaml value,
 either an Abstract tag, or Nativeint, see the manual
 https://ocaml.org/manual/intfc.html#ss:c-outside-head)

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
---
 tools/ocaml/libs/eventchn/xeneventchn_stubs.c | 29 +++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/tools/ocaml/libs/eventchn/xeneventchn_stubs.c b/tools/ocaml/libs/eventchn/xeneventchn_stubs.c
index f889a7a2e4..c0d57e2954 100644
--- a/tools/ocaml/libs/eventchn/xeneventchn_stubs.c
+++ b/tools/ocaml/libs/eventchn/xeneventchn_stubs.c
@@ -33,7 +33,30 @@
 #include <caml/fail.h>
 #include <caml/signals.h>
 
-#define _H(__h) ((xenevtchn_handle *)(__h))
+/* We want to close the event channel when it is no longer in use,
+   which can only be done safely with a finalizer.
+   Event channels are typically long lived, so we don't need tighter control over resource deallocation.
+   Use a custom block
+*/
+
+/* Access the xenevtchn_t* part of the OCaml custom block */
+#define _H(__h) (*((xenevtchn_handle**)Data_custom_val(__h)))
+
+static void stub_evtchn_finalize(value v)
+{
+	/* docs say to not use any CAMLparam* macros here */
+	xenevtchn_close(_H(v));
+}
+
+static struct custom_operations xenevtchn_ops = {
+	"xenevtchn",
+	stub_evtchn_finalize,
+	custom_compare_default, /* raises Failure, cannot compare */
+	custom_hash_default, /* ignored */
+	custom_serialize_default, /* raises Failure, can't serialize */
+	custom_deserialize_default, /* raises Failure, can't deserialize */
+	custom_compare_ext_default /* raises Failure */
+};
 
 CAMLprim value stub_eventchn_init(void)
 {
@@ -48,7 +71,9 @@ CAMLprim value stub_eventchn_init(void)
 	if (xce == NULL)
 		caml_failwith("open failed");
 
-	result = (value)xce;
+	/* contains file descriptors, trigger full GC at least every 128 allocations */
+	result = caml_alloc_custom(&xenevtchn_ops, sizeof(xce), 1, 128);
+	_H(result) = xce;
 	CAMLreturn(result);
 }
 
-- 
2.34.1

Re: [PATCH v1 7/7] tools/ocaml/libs/eventchn: do not leak event channels and OCaml 5.0 compat

[Christian Lindig]

[-- Attachment #1: Type: text/plain, Size: 740 bytes --]



On 29 Jul 2022, at 18:53, Edwin Török <edvin.torok@citrix.com<mailto:edvin.torok@citrix.com>> wrote:

Add a finalizer on the event channel value, so that it calls
`xenevtchn_close` when the value would be GCed.

In practice oxenstored seems to be the only user of this,
and it creates a single global event channel only,
but freeing this could still be useful when run with OCAMLRUNPARAM=c

The code was previously casting a C pointer to an OCaml value,
which should be avoided: OCaml 5.0 won't support it.
(all "naked" C pointers must be wrapped inside an OCaml value,
either an Abstract tag, or Nativeint, see the manual

Acked-by: Christian Lindig <christian.lindig@citrix.com<mailto:christian.lindig@citrix.com>>


[-- Attachment #2: Type: text/html, Size: 8058 bytes --]

Re: [PATCH v1 7/7] tools/ocaml/libs/eventchn: do not leak event channels and OCaml 5.0 compat

[Andrew Cooper]

On 29/07/2022 18:53, Edwin Török wrote:
> Add a finalizer on the event channel value, so that it calls
> `xenevtchn_close` when the value would be GCed.
>
> In practice oxenstored seems to be the only user of this,
> and it creates a single global event channel only,
> but freeing this could still be useful when run with OCAMLRUNPARAM=c
>
> The code was previously casting a C pointer to an OCaml value,
> which should be avoided: OCaml 5.0 won't support it.
> (all "naked" C pointers must be wrapped inside an OCaml value,
>  either an Abstract tag, or Nativeint, see the manual
>  https://ocaml.org/manual/intfc.html#ss:c-outside-head)
>
> Signed-off-by: Edwin Török <edvin.torok@citrix.com>

So while this looks like a good improvement, don't we need to do it for
all libraries, not just evtchn?

It doesn't look as if Ocaml 5.0 is very far away.

> ---
>  tools/ocaml/libs/eventchn/xeneventchn_stubs.c | 29 +++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/tools/ocaml/libs/eventchn/xeneventchn_stubs.c b/tools/ocaml/libs/eventchn/xeneventchn_stubs.c
> index f889a7a2e4..c0d57e2954 100644
> --- a/tools/ocaml/libs/eventchn/xeneventchn_stubs.c
> +++ b/tools/ocaml/libs/eventchn/xeneventchn_stubs.c
> @@ -33,7 +33,30 @@
>  #include <caml/fail.h>
>  #include <caml/signals.h>
>  
> -#define _H(__h) ((xenevtchn_handle *)(__h))
> +/* We want to close the event channel when it is no longer in use,
> +   which can only be done safely with a finalizer.
> +   Event channels are typically long lived, so we don't need tighter control over resource deallocation.
> +   Use a custom block
> +*/
> +
> +/* Access the xenevtchn_t* part of the OCaml custom block */
> +#define _H(__h) (*((xenevtchn_handle**)Data_custom_val(__h)))
> +
> +static void stub_evtchn_finalize(value v)
> +{
> +	/* docs say to not use any CAMLparam* macros here */
> +	xenevtchn_close(_H(v));
> +}
> +
> +static struct custom_operations xenevtchn_ops = {
> +	"xenevtchn",
> +	stub_evtchn_finalize,
> +	custom_compare_default, /* raises Failure, cannot compare */
> +	custom_hash_default, /* ignored */
> +	custom_serialize_default, /* raises Failure, can't serialize */
> +	custom_deserialize_default, /* raises Failure, can't deserialize */
> +	custom_compare_ext_default /* raises Failure */

This wants to gain a trailing comma.

> +};
>  
>  CAMLprim value stub_eventchn_init(void)
>  {
> @@ -48,7 +71,9 @@ CAMLprim value stub_eventchn_init(void)
>  	if (xce == NULL)
>  		caml_failwith("open failed");
>  
> -	result = (value)xce;
> +	/* contains file descriptors, trigger full GC at least every 128 allocations */
> +	result = caml_alloc_custom(&xenevtchn_ops, sizeof(xce), 1, 128);

The memory allocated for xce is tiny (48 bytes) and invariant for the
lifetime of the evtchn object, which we've already established tends to
operate as a singleton anyway.

Don't we want to use the recommended 0 and 1 here?

~Andrew

Re: [PATCH v1 7/7] tools/ocaml/libs/eventchn: do not leak event channels and OCaml 5.0 compat

[Edwin Torok]

> On 5 Aug 2022, at 19:06, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
> 
> On 29/07/2022 18:53, Edwin Török wrote:
>> Add a finalizer on the event channel value, so that it calls
>> `xenevtchn_close` when the value would be GCed.
>> 
>> In practice oxenstored seems to be the only user of this,
>> and it creates a single global event channel only,
>> but freeing this could still be useful when run with OCAMLRUNPARAM=c
>> 
>> The code was previously casting a C pointer to an OCaml value,
>> which should be avoided: OCaml 5.0 won't support it.
>> (all "naked" C pointers must be wrapped inside an OCaml value,
>> either an Abstract tag, or Nativeint, see the manual
>> https://ocaml.org/manual/intfc.html#ss:c-outside-head)
>> 
>> Signed-off-by: Edwin Török <edvin.torok@citrix.com>
> 
> So while this looks like a good improvement, don't we need to do it for
> all libraries, not just evtchn?
> 
> It doesn't look as if Ocaml 5.0 is very far away.

There are 2 ways to make it safe: use a block with Abstract_tag, or a block with Custom_tag:
https://v2.ocaml.org/manual/intfc.html#ss:c-outside-head

(technically it only ever was safe to use naked pointers for word-aligned pointers previously, although malloc usually
 ensures some minimal alignment).

There is a mode where the runtime can warn on stderr whenever naked pointers are used (there is an opam switch for it,
or one can specify a flag during the compiler build time), but it only does so when that codepath is hit,
not at build/link time.

A quick look at the libs:
* libs/mmap: uses Abstract_tag
* eventchn: fixed here to use Custom_tag
* libs/xc: needs fixing, stub_xc_interface_open
* libs/xl: uses Custom_tag (although it has other known issues (it should never use caml_callbackN directly, but use caml_callbackN_exn, see https://v2.ocaml.org/manual/intfc.html#ss:c-callbacks)
* libs/xentoollog: uses Custom_tag

So we need a patch for libs/xc.

> 
>> ---
>> tools/ocaml/libs/eventchn/xeneventchn_stubs.c | 29 +++++++++++++++++--
>> 1 file changed, 27 insertions(+), 2 deletions(-)
>> 
>> diff --git a/tools/ocaml/libs/eventchn/xeneventchn_stubs.c b/tools/ocaml/libs/eventchn/xeneventchn_stubs.c
>> index f889a7a2e4..c0d57e2954 100644
>> --- a/tools/ocaml/libs/eventchn/xeneventchn_stubs.c
>> +++ b/tools/ocaml/libs/eventchn/xeneventchn_stubs.c
>> @@ -33,7 +33,30 @@
>> #include <caml/fail.h>
>> #include <caml/signals.h>
>> 
>> -#define _H(__h) ((xenevtchn_handle *)(__h))
>> +/* We want to close the event channel when it is no longer in use,
>> +   which can only be done safely with a finalizer.
>> +   Event channels are typically long lived, so we don't need tighter control over resource deallocation.
>> +   Use a custom block
>> +*/
>> +
>> +/* Access the xenevtchn_t* part of the OCaml custom block */
>> +#define _H(__h) (*((xenevtchn_handle**)Data_custom_val(__h)))
>> +
>> +static void stub_evtchn_finalize(value v)
>> +{
>> +	/* docs say to not use any CAMLparam* macros here */
>> +	xenevtchn_close(_H(v));
>> +}
>> +
>> +static struct custom_operations xenevtchn_ops = {
>> +	"xenevtchn",
>> +	stub_evtchn_finalize,
>> +	custom_compare_default, /* raises Failure, cannot compare */
>> +	custom_hash_default, /* ignored */
>> +	custom_serialize_default, /* raises Failure, can't serialize */
>> +	custom_deserialize_default, /* raises Failure, can't deserialize */
>> +	custom_compare_ext_default /* raises Failure */
> 
> This wants to gain a trailing comma.
> 
>> +};
>> 
>> CAMLprim value stub_eventchn_init(void)
>> {
>> @@ -48,7 +71,9 @@ CAMLprim value stub_eventchn_init(void)
>> 	if (xce == NULL)
>> 		caml_failwith("open failed");
>> 
>> -	result = (value)xce;
>> +	/* contains file descriptors, trigger full GC at least every 128 allocations */
>> +	result = caml_alloc_custom(&xenevtchn_ops, sizeof(xce), 1, 128);
> 
> The memory allocated for xce is tiny (48 bytes) and invariant for the
> lifetime of the evtchn object, which we've already established tends to
> operate as a singleton anyway.
> 
> Don't we want to use the recommended 0 and 1 here?

It is not just about the memory itself, but also about the file descriptors: those are a limited resource,
and if we use the 0 and 1 it means that this will be garbage collected very infrequently since the allocation itself is very small,
and you could potentially run out of file descriptors if you keep opening new event channels.
Notice there is no API for the user to close the event channel, so it has to rely on the GC, which is not ideal.

The mirage version of the eventchn lib does provide a close function: https://github.com/mirage/ocaml-evtchn/blob/main/lib/eventchn.mli,
although its implementation just leaks it (to avoid use-after-free): https://github.com/mirage/ocaml-evtchn/blob/main/lib/eventchn.ml#L42

Are event channel always expected to be singletons, is there a valid use case where you'd want more than 1 event channel/process?

Best regards,
--Edwin

Re: [PATCH v1 7/7] tools/ocaml/libs/eventchn: do not leak event channels and OCaml 5.0 compat

[Andrew Cooper]

On 08/08/2022 09:28, Edwin Torok wrote:
>> On 5 Aug 2022, at 19:06, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
>>
>> On 29/07/2022 18:53, Edwin Török wrote:
>>> +};
>>>
>>> CAMLprim value stub_eventchn_init(void)
>>> {
>>> @@ -48,7 +71,9 @@ CAMLprim value stub_eventchn_init(void)
>>> 	if (xce == NULL)
>>> 		caml_failwith("open failed");
>>>
>>> -	result = (value)xce;
>>> +	/* contains file descriptors, trigger full GC at least every 128 allocations */
>>> +	result = caml_alloc_custom(&xenevtchn_ops, sizeof(xce), 1, 128);
>> The memory allocated for xce is tiny (48 bytes) and invariant for the
>> lifetime of the evtchn object, which we've already established tends to
>> operate as a singleton anyway.
>>
>> Don't we want to use the recommended 0 and 1 here?
> It is not just about the memory itself, but also about the file descriptors: those are a limited resource,
> and if we use the 0 and 1 it means that this will be garbage collected very infrequently since the allocation itself is very small,
> and you could potentially run out of file descriptors if you keep opening new event channels.
> Notice there is no API for the user to close the event channel, so it has to rely on the GC, which is not ideal.
>
> The mirage version of the eventchn lib does provide a close function: https://github.com/mirage/ocaml-evtchn/blob/main/lib/eventchn.mli,
> although its implementation just leaks it (to avoid use-after-free): https://github.com/mirage/ocaml-evtchn/blob/main/lib/eventchn.ml#L42
>
> Are event channel always expected to be singletons, is there a valid use case where you'd want more than 1 event channel/process?

Careful on terminology.  We're discussing an open /dev/xen/evtchn file
handle, upon which an arbitrary number of event channels are muliplexed.

There's no good reason to have more than one file handle open, and
there's no good reason to close it except during shutdown.

So 0 and 1 are probably the right values in this case.

~Andrew

Re: [PATCH v1 0/7] tools/ocaml code and build cleanups

[Christian Lindig]

[-- Attachment #1: Type: text/plain, Size: 3443 bytes --]



On 29 Jul 2022, at 18:53, Edwin Török <edvin.torok@citrix.com<mailto:edvin.torok@citrix.com>> wrote:

Various OCaml code cleanups to make building and working on Oxenstored easier,
including compatibility with newer language versions.
This does not yet change the minimum version of OCaml.

A version of this series in a git repository is publicly available at:
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fedwintorok%2Fxen.git&amp;data=05%7C01%7Cchristian.lindig%40citrix.com%7C0e39bbb48174454226b008da718b57ff%7C335836de42ef43a2b145348c2ee9ca5b%7C0%7C0%7C637947140547020875%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=KsQgF0l%2FDzSxyIkvG7t15wGfHtNlG5VMJuinI5eZ4AM%3D&amp;reserved=0
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fedwintorok%2Fxen%2Fcompare%2Fprivate%2Fedvint%2Fpublic%3Fexpand%3D1&amp;data=05%7C01%7Cchristian.lindig%40citrix.com%7C0e39bbb48174454226b008da718b57ff%7C335836de42ef43a2b145348c2ee9ca5b%7C0%7C0%7C637947140547020875%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=sHJfI4P4aUSdUVFpbo3NNzkdfzOgXejRSI%2FNicHQ750%3D&amp;reserved=0

Edwin Török (7):
tools/ocaml/Makefile: do not run ocamldep during make clean
tools/ocaml/*/Makefile: generate paths.ml from configure
tools/ocaml/*/dune: dune based build system
tools/ocaml: Makefile to drive dune
tools/ocaml: fix compiler warnings
tools/ocaml/libs/xb: hide type of Xb.t
tools/ocaml/libs/eventchn: do not leak event channels and OCaml 5.0
compat

Makefile | 5 ++
tools/.gitignore | 7 ++
tools/configure | 4 +-
tools/configure.ac | 2 +
tools/dune | 5 ++
tools/dune-project | 1 +
tools/ocaml/Makefile.dune | 88 +++++++++++++++++++
tools/ocaml/Makefile.rules | 2 +
tools/ocaml/dune-project | 27 ++++++
tools/ocaml/dune-workspace.dev.in | 2 +
tools/ocaml/dune-workspace.in | 18 ++++
tools/ocaml/libs/eventchn/dune | 11 +++
tools/ocaml/libs/eventchn/xeneventchn_stubs.c | 29 +++++-
tools/ocaml/libs/mmap/dune | 9 ++
tools/ocaml/libs/xb/dune | 10 +++
tools/ocaml/libs/xb/xb.ml | 3 +
tools/ocaml/libs/xb/xb.mli | 9 +-
tools/ocaml/libs/xc/dune | 16 ++++
tools/ocaml/libs/xs/Makefile | 5 --
tools/ocaml/libs/xs/dune | 15 ++++
tools/ocaml/libs/xs/paths.ml.in | 1 +
tools/ocaml/xenstored/Makefile | 5 --
tools/ocaml/xenstored/connection.ml | 10 +--
tools/ocaml/xenstored/dune | 51 +++++++++++
tools/ocaml/xenstored/paths.ml.in | 4 +
tools/ocaml/xenstored/process.ml | 5 +-
26 files changed, 315 insertions(+), 29 deletions(-)
create mode 100644 tools/.gitignore
create mode 100644 tools/dune
create mode 100644 tools/dune-project
create mode 100644 tools/ocaml/Makefile.dune
create mode 100644 tools/ocaml/dune-project
create mode 100644 tools/ocaml/dune-workspace.dev.in
create mode 100644 tools/ocaml/dune-workspace.in
create mode 100644 tools/ocaml/libs/eventchn/dune
create mode 100644 tools/ocaml/libs/mmap/dune
create mode 100644 tools/ocaml/libs/xb/dune
create mode 100644 tools/ocaml/libs/xc/dune
create mode 100644 tools/ocaml/libs/xs/dune
create mode 100644 tools/ocaml/libs/xs/paths.ml.in
create mode 100644 tools/ocaml/xenstored/dune
create mode 100644 tools/ocaml/xenstored/paths.ml.in

Acked-by: Christian Lindig <christian.lindig@citrix.com<mailto:christian.lindig@citrix.com>>



[-- Attachment #2: Type: text/html, Size: 46669 bytes --]