Use Lwt.Syntax and avoid some >>= fun () patterns

[dinosaure]

This little commit takes the advantage of let* to replace some parts of the code. In my opinion (feel free to take it in account), the code is more readable than before. About the vifs function, I take the advantage that String.split_on_char can not return an empty list according to the documentation: we can pattern-match with [@warning "-8"] or use List.hd.

[hannesm]

looks pretty good, some minor remarks inline

[palainp]

Thank you @dinosaure I just tested and it works as before. The CI failure is just about the resulting hashsum mismatch.

I would suggest, if you have other improvements to add them all and change the hashsum in build-with.sh at the end (we can cut a release when merged as the hashsum in build-with.sh in main is checked in the salt script).

[dinosaure]

Just to inform you, I continue to look deeper into the code and see many places where we can improve/upgrade the code :+1:. I will continue to propose some PRs about that: don't think that this is the only and last PR :+1:.

[palainp]

Just to inform you, I continue to look deeper into the code and see many places where we can improve/upgrade the code 👍. I will continue to propose some PRs about that: don't think that this is the only and last PR 👍.

Or continue into this one? :D

[dinosaure]

Or continue into this one? :D

As you want if it's clearer for you.

[hannesm]

I think due to the checksum / release stuff, there are two or three options (as I don't quite know about the salt stuff):

• continue refactorings in this PR (and then maybe squash the existing commits into a single one)
• merge to a dev branch, which at the end we'll merge into the main branch and cut a release
• fix the salt script to use the latest release instead of the main branch (if this is what is happening)

It would be great to keep the checksum as produced by CI and the one in the build-with file in sync... so I guess the first option is a good one :)

[hannesm]

Of course, it is as well fine to accumulate commits here and then do a squash-merge :D

[palainp]

I think due to the checksum / release stuff, there are two or three options (as I don't quite know about the salt stuff):

• continue refactorings in this PR (and then maybe squash the existing commits into a single one)
• merge to a dev branch, which at the end we'll merge into the main branch and cut a release
• fix the salt script to use the latest release instead of the main branch (if this is what is happening)

I think solution (a) is the easiest so far, and (c) should be done at some point, and maybe it's easier to include the vmlinuz hashsum in the .sha256 file in the release (but TBH I'm unsure checking the vmlinuz is mandatory if the .bz2 hash already matches?, and https://github.com/unman/shaker/tree/main/mirage only check the .bz2 sum).

It would be great to keep the checksum as produced by CI and the one in the build-with file in sync... so I guess the first option is a good one :)

I also agree that having the correct hashsum in build-with.sh is important to avoid weekly CI failures :)

[dinosaure]

My last patch can be a bit controversial and we should stop at this point to see if the unikernel works again. I explained in the commit why I did this change. Let me know if it's ok or not.

[palainp]

My last patch can be a bit controversial and we should stop at this point to see if the unikernel works again. I explained in the commit why I did this change. Let me know if it's ok or not.

Good news, at this point, qmf is still working (not a deep test: 3 clients in and out, bandwidth speed is the same as before). I'm probably ok with that change (at least I still understand the code), but I'm interested with others' thoughts :)

[hannesm]

I like the last commit, thanks @dinosaure.

It would make reviewing easier if you could separate syntactic changes (using Lwt.Syntax, using @@) and semantic changes.

I added some comments, and thanks to @palainp for testing this. It is intricate code where we should ensure to test extensively before merging (with adding and removing clients). Maybe I'm also afraid since I spent quite some time on that code, and was sometimes in stuck states... But as said, I reviewed the code and it looks good.

Should we adapt ocamlformat to this repository, so we don't have to argue about indentation, where to put parenthesis and where to put the in?

[palainp]

Unfortunately I don't have yet a deep test protocol, all is done manually, and I may miss something, but I'll try to write a testbed script soon :)

Should we adapt ocamlformat to this repository, so we don't have to argue about indentation, where to put parenthesis and where to put the in?

Yes I think so :)

[palainp]

I wrote a simple naive script (using 10 AppVM clients with 2G memory) that returned me some issues (certainly present before this PR). When I change the netvm property I have in the logs (maybe a packet is still pending in the ring when the unikernel tries to connect to a new netvm?):

[2024-05-23 10:31:14] 2024-05-23T08:31:14-00:00: [ERROR] [dispatcher] uncaught exception trying to send to uplink: 
[2024-05-23 10:31:14]                                                 Failure("Shared_page_pool.use after shutdown")

I also got in dom0 console:

2024-05-23 10:27:08.738 qrexec-client[58505]: process_io.c:188:process_io: vchan connection closed early (fds: 1 -1 -1, status: -1 -1)

but that's more related to my script to be to eager when connecting/disconnecting clients AppVMs. I'll have to rewrite that before sharing :)

[hannesm]

I'd like to ask what to do here. I'm happy to rebase this PR and if @palainp tests we merge it!? It'd esp. be wonderful to have it included before the next release, so we get something out of the nice cleanups here :) Unfortunately it conflicts now that #201 was merged.

[hannesm]

I rebased on top of main, cleanup up a whitespace and revised XXX to NOTE.

The leftover comment is about "@@", but a git grep "@@" results in several hits, so let's leave that coding style for a separate PR.

[palainp]

Thank you! I need to test it, and think this PR will be good to merge!