Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wait the backend-id has been created by Xen before trying to read it #107

Merged
merged 2 commits into from
Jul 22, 2023

Conversation

palainp
Copy link
Member

@palainp palainp commented Jul 12, 2023

This is needed to avoid an exception when we want to connect the uplink (See #106 (reopen and discussion there?) and mirage/qubes-mirage-firewall#156 for details. I'm now facing that issue when I want to be ablke to dynamically change the qubes-mirage-firewall netvm, see https://github.com/palainp/qubes-mirage-firewall/tree/common-vif).

Steps to reproduce are like (in the context of Qubes, but I assume there's something similar with plain Xen):

  • connect to backend Netif.connect "0" >>= fun net ->
  • change the netvm in Qubes (that will change our backend and we need to disconnect and reconnect)
  • Disconnect Netif.disconnect net;
  • And reconnect as step 1

That will trigger a Enoent exception (it seems that the backend-id key is not written fast enough) from xenstore.ml:265:

Fatal error: exception Xs_protocol.Enoent("read")
Raised at Xs_protocol.response in file "duniverse/ocaml-xenstore/core/xs_protocol.ml", line 681, characters 20-39
Called from Xs_client_lwt.Client.rpc.(fun) in file "duniverse/ocaml-xenstore/client_lwt/xs_client_lwt.ml", line 318, characters 13-50
Called from Lwt.Sequential_composition.bind.create_result_promise_and_callback_if_deferred.callback in file "duniverse/lwt/src/core/lwt.ml", line 1849, characters 23-26
Re-raised at Lwt.Miscellaneous.poll in file "duniverse/lwt/src/core/lwt.ml", line 3077, characters 20-29
Called from Xen_os__Main.run.aux in file "duniverse/mirage-xen/lib/main.ml", line 37, characters 10-20
Called from Dune__exe__Main.run in file "main.ml" (inlined), line 3, characters 12-29
Called from Dune__exe__Main in file "main.ml", line 114, characters 5-10
Solo5: solo5_exit(2) called

With that PR I don't see anymore the exception, but maybe there is other places to put that "wait before read"? Or maybe it should be a default behavior and read becomes read_unsafe?

Copy link
Member

@djs55 djs55 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

To be honest I've never been sure exactly when various keys are written (and whether they are written in a transaction with other keys or not).

I think it's a reasonable approach to use a xenstore watch here.

@palainp
Copy link
Member Author

palainp commented Jul 14, 2023

Dear @djs55 , thanks for your comment!

In the last commit I fixed the CI errors, and I'll leave the choice of extending safe_read to other reads for later, for now I want to be conservative :)

@palainp palainp merged commit f431ba7 into mirage:main Jul 22, 2023
@palainp palainp deleted the wait-before-read branch July 22, 2023 15:31
palainp added a commit to palainp/opam-repository that referenced this pull request Jul 22, 2023
CHANGES:

* xenstore: read_backend now waits for the backend-id key to be written by
  Xen before reading it to avoid raising an exception (@palainp, mirage/mirage-net-xen#107)
palainp added a commit to palainp/opam-repository that referenced this pull request Jul 23, 2023
CHANGES:

* xenstore: read_backend now waits for the backend-id key to be written by
  Xen before reading it to avoid raising an exception (@palainp, mirage/mirage-net-xen#107)
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
CHANGES:

* xenstore: read_backend now waits for the backend-id key to be written by
  Xen before reading it to avoid raising an exception (@palainp, mirage/mirage-net-xen#107)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants