Use Lwt.Syntax and avoid some >>= fun () patterns #197
Conversation
|
looks pretty good, some minor remarks inline |
|
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 |
|
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 |
As you want if it's clearer for you. |
|
I think due to the checksum / release stuff, there are two or three options (as I don't quite know about the salt stuff):
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 :) |
|
Of course, it is as well fine to accumulate commits here and then do a squash-merge :D |
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
I also agree that having the correct hashsum in |
|
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 :) |
| let* () = | ||
| Lwt.catch (add_vif get_ts vif dns_client dns_servers ~client_ip ~router | ||
| ~cleanup_tasks qubesDB) | ||
| @@ fun exn -> |
There was a problem hiding this comment.
I personally struggle with @@ and find parenthesis much easier to read (since I'm always unsure how strong the @@ binding strength is.
| clients := !clients |> Dao.VifMap.add key cleanup))) | ||
| let open Lwt.Syntax in | ||
| let clients : Cleanup.t Dao.VifMap.t ref = ref Dao.VifMap.empty in | ||
| Dao.watch_clients @@ fun new_set -> |
|
I like the last commit, thanks @dinosaure. It would make reviewing easier if you could separate syntactic changes (using Lwt.Syntax, using 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 |
|
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 :)
Yes I think so :) |
|
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?): I also got in dom0 console: 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 :) |
…nce ipaddr.5.2.0)
…clients map We currently try to spawn 2 fibers [qubes_updated] and [listener] per clients and we already finalise them correctly if the client is disconnected. However, the Lwt.async is localized into add_client instead of where we attach a finalisers for these tasks. The first objective of this patch is to be sure that the Lwt.async is near where we registerd cancellation of these tasks. The second part is to localize the global clients to avoid the ability to read/write on it somewhere else. Only Dispatcher.watch_clients uses it - so it corresponds to a free variable of the Dispatcher.watch_clients closure.
|
I rebased on top of main, cleanup up a whitespace and revised XXX to NOTE. The leftover comment is about "@@", but a |
|
Thank you! I need to test it, and think this PR will be good to merge! |
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 thevifsfunction, I take the advantage thatString.split_on_charcan not return an empty list according to the documentation: we can pattern-match with[@warning "-8"]or useList.hd.