[for-4.9,v2,0/3] oxenstored: make it work on FreeBSD

Commit Message

Christian Lindig April 19, 2017, 12:56 p.m. UTC

> On 18. Apr 2017, at 16:31, Wei Liu <[e-mail]> wrote:
> 
> Wei Liu (3):
>  paths.m4: provide XENSTORED_{KVA,PORT}
>  oxenstored: provide options to define xenstored devices
>  hotplug/FreeBSD: configure xenstored
> 
> m4/paths.m4                              | 12 ++++++++++++
> tools/hotplug/FreeBSD/rc.d/xencommons.in |  8 +++++---
> tools/ocaml/xenstored/define.ml          |  3 ---
> tools/ocaml/xenstored/domains.ml         |  7 +++++--
> tools/ocaml/xenstored/oxenstored.conf.in |  3 +++
> tools/ocaml/xenstored/xenstored.ml       |  4 +++-
> 6 files changed, 28 insertions(+), 9 deletions(-)
> 
> -- 
> 2.11.

This approach adds two new entries into oxenstored.conf that are determined by the configure script. I prefer it over the previous design not the least because it results in a much smaller change and doesn’t require new bindings for C libraries. The code looks good and I believe it doesn’t change in significant ways in its failure modes as port and fd are still read from files - just the names of these files are now coming from oxenstored.conf. I’m not sure the paths.m4 is taking the best approach by relying on $host_os rather than testing the paths but I would leave that to people with more autoconf experience to comment on.

---
Rerun autogen.sh
---
m4/paths.m4 | 12 ++++++++++++
1 file changed, 12 insertions(+)

-- 
2.11.0

I’d be fine with taking this patch.

— Christian

Comments

Ian Jackson April 20, 2017, 10:22 a.m. UTC | #1

Christian Lindig writes ("Re: [PATCH for-4.9 v2 0/3] oxenstored: make it work on FreeBSD"):
> This approach adds two new entries into oxenstored.conf that are
> determined by the configure script. I prefer it over the previous
> design not the least because it results in a much smaller change and
> doesn’t require new bindings for C libraries. The code looks good
> and I believe it doesn’t change in significant ways in its failure
> modes as port and fd are still read from files - just the names of
> these files are now coming from oxenstored.conf. I’m not sure the
> paths.m4 is taking the best approach by relying on $host_os rather
> than testing the paths but I would leave that to people with more
> autoconf experience to comment on.

Let me repeat something I said in the corridor: we mustn't test for
the existence of these paths at build-time, because the build host
might not be running Xen.

Personally I would have just tested both paths at runtime, but this
configure-based approach is fine too.

> +case "$host_os" in
> +*freebsd*) XENSTORED_KVA=/dev/xen/xenstored ;;
> +*) XENSTORED_KVA=/proc/xen/xsd_kva ;;
> +esac
> +AC_SUBST(XENSTORED_KVA)
> +
> +case "$host_os" in
> +*freebsd*) XENSTORED_PORT=/dev/xen/xenstored ;;
> +*) XENSTORED_PORT=/proc/xen/xsd_port ;;
> +esac
> +AC_SUBST(XENSTORED_PORT)

I'm not sure why one wouldn't combine these two case statements, but I
don't care enough to suggest reworking it.

All three patches:

Acked-by: Ian Jackson <[e-mail]>

Thanks,
Ian.

Julien Grall April 20, 2017, 12:50 p.m. UTC | #2

Hi,

On 20/04/17 11:22, Ian Jackson wrote:
> Christian Lindig writes ("Re: [PATCH for-4.9 v2 0/3] oxenstored: make it work on FreeBSD"):
>> This approach adds two new entries into oxenstored.conf that are
>> determined by the configure script. I prefer it over the previous
>> design not the least because it results in a much smaller change and
>> doesn’t require new bindings for C libraries. The code looks good
>> and I believe it doesn’t change in significant ways in its failure
>> modes as port and fd are still read from files - just the names of
>> these files are now coming from oxenstored.conf. I’m not sure the
>> paths.m4 is taking the best approach by relying on $host_os rather
>> than testing the paths but I would leave that to people with more
>> autoconf experience to comment on.
>
> Let me repeat something I said in the corridor: we mustn't test for
> the existence of these paths at build-time, because the build host
> might not be running Xen.
>
> Personally I would have just tested both paths at runtime, but this
> configure-based approach is fine too.
>
>> +case "$host_os" in
>> +*freebsd*) XENSTORED_KVA=/dev/xen/xenstored ;;
>> +*) XENSTORED_KVA=/proc/xen/xsd_kva ;;
>> +esac
>> +AC_SUBST(XENSTORED_KVA)
>> +
>> +case "$host_os" in
>> +*freebsd*) XENSTORED_PORT=/dev/xen/xenstored ;;
>> +*) XENSTORED_PORT=/proc/xen/xsd_port ;;
>> +esac
>> +AC_SUBST(XENSTORED_PORT)
>
> I'm not sure why one wouldn't combine these two case statements, but I
> don't care enough to suggest reworking it.
>
> All three patches:
>
> Acked-by: Ian Jackson <[e-mail]>

Release-acked-by: Julien Grall <[e-mail]>

Cheers,

Patch

diff --git a/m4/paths.m4 b/m4/paths.m4
index 93ce89ab40..f208b7e39f 100644
--- a/m4/paths.m4
+++ b/m4/paths.m4
@@ -147,3 +147,15 @@  AC_SUBST(XEN_PAGING_DIR)
XEN_DUMP_DIR=$xen_dumpdir_path
AC_SUBST(XEN_DUMP_DIR)
])
+
+case "$host_os" in
+*freebsd*) XENSTORED_KVA=/dev/xen/xenstored ;;
+*) XENSTORED_KVA=/proc/xen/xsd_kva ;;
+esac
+AC_SUBST(XENSTORED_KVA)
+
+case "$host_os" in
+*freebsd*) XENSTORED_PORT=/dev/xen/xenstored ;;
+*) XENSTORED_PORT=/proc/xen/xsd_port ;;
+esac
+AC_SUBST(XENSTORED_PORT)