search

xapi-project / xapi-nbd

Expose XenServer disks conveniently over NBD
Other
4 stars 11 forks source link

CP-23829: Preliminary TLS functionality #13

Closed thomassa closed 7 years ago

thomassa commented 7 years ago

It works but some code is copied from the nbd repository, rather than being shared.

thomassa commented 7 years ago

Depends on xapi-project/nbd#79 which adds TLS functionality to the nbd package.

mseri commented 7 years ago

The build is failing with

- File "src/main.ml", line 126, characters 2-27:
- Error: Unbound value Nbd_lwt_unix.with_channel
thomassa commented 7 years ago

Also with this change, xapi-nbd requires at least one command-line argument, so in the xenserver-specs repository's xapi-nbd.service file for systemd, the ExecStart=/usr/sbin/xapi-nbd start-up command will need to cater for this.

mseri commented 7 years ago

Can we make the parameter optional, falling back to the old behaviour?

thomassa commented 7 years ago

@mseri wrote

The build is failing with Error: Unbound value Nbd_lwt_unix.with_channel

Yes, that's expected because it depends on the change in the nbd package.

thomassa commented 7 years ago

I'm reluctant to fall back to the old behaviour because that would be not using TLS. (I suppose we could implement the fallback temporarily, so that we can change repositories one at a time rather than needing to synchronise the changes.)

mseri commented 7 years ago

Let's review and merge nbd first and then this one, so that we don't need the fallback

thomassa commented 7 years ago

I think it's best to review this and the nbd pull-request together so that the review includes the new/changed parts of the interface between them.

mseri commented 7 years ago

Please add the fix for travis. It should be enough to use 4.04.2 as the compiler

thomassa commented 7 years ago

I've added a second commit to use Server.with_connection though really that's unrelated to the addition of TLS functionality. Now to change the compiler to make Travis happy...

mseri commented 7 years ago

I think with the script we use here it is version 4.04 :-/

thomassa commented 7 years ago

Trying with 4.04 now... Hah. I see it's busy installing 4.04.2 anyway.

mseri commented 7 years ago

It's working. It is now failing only because we have not released the new nbd

thomassa commented 7 years ago

@mseri ...and that new release is defined in https://github.com/xapi-project/xs-opam/pull/134

mseri commented 7 years ago

Yes, I saw that. I think this can be merged.