-
Notifications
You must be signed in to change notification settings - Fork 20
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
Use new RPC mechanism #53
Conversation
src/squeezed.ml
Outdated
S.delete_reservation delete_reservation; | ||
S.transfer_reservation_to_domain transfer_reservation_to_domain; | ||
S.query_reservation_of_domain query_reservation_of_domain; | ||
S.balance_memory balance_memory; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You got spaces instead of tabs here.
src/squeezed.ml
Outdated
@@ -29,17 +29,33 @@ let options = [ | |||
"domain-zero-dynamic-max", Arg.String (fun x -> Squeeze.domain_zero_dynamic_max := if x = "auto" then None else Some (Int64.of_string x)), (fun () -> match !Squeeze.domain_zero_dynamic_max with None -> "using the static-max value" | Some x -> Int64.to_string x), "Maximum memory to allow domain 0"; | |||
] | |||
|
|||
module S=Memory_interface.API(Idl.GenServerExn ()) | |||
|
|||
let bind () = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being this the first PR using the new ppx mechanism, I think it would be worth to document line 32 and the content of 34. Just to make it more explicit that we are 'binding' the declared calls in the idl with the implementations provided in memory_server
.
src/memory_server.ml
Outdated
@@ -45,9 +45,9 @@ let wrap dbg f = | |||
(* NB both needed and free have been inflated by the lowmem_emergency_pool etc *) | |||
let needed = Int64.sub needed Squeeze_xen.target_host_free_mem_kib | |||
and free = Int64.sub free Squeeze_xen.target_host_free_mem_kib in | |||
raise (Memory_interface.Cannot_free_this_much_memory (needed, free)) | |||
raise (MemoryError (Memory_interface.Cannot_free_this_much_memory (needed, free))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to return the error and use the err
construct to automatically decide if we are raising errors or returning Error
s?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not automatically, but you can do it. You use GenClient
or GenServer
rather than GenClientExn
or GenServerExn
. For example, if you declare your errors like this:
type errors =
| MyError1 [@doc ["Error1 doc"]]
| MyError2 [@doc ["Error2 doc"]]
[@@deriving rpcty]
exception ErrorExn of errors
let err = Error.{
def = errors;
raiser = (function | e -> raise (ErrorExn e));
matcher = function | ErrorExn e -> Some e | _ -> None
}
and your method like this:
let method = declare "test" ["A test method"] (int @-> string @-> returning int err)
then when you use GenServerExn
, the expected signature of the implementation will be
test: int -> string -> int
and it will correctly marshal the error if you raise ErrorExn
in the middle of your implementation.
However, if you use GenServer
, the expected type of the implementation will be
test: int -> string -> (int, errors) Result.result
src/squeezed.ml
Outdated
let _ = | ||
debug "squeezed version %d.%d starting" major_version minor_version; | ||
|
||
configure ~options (); | ||
|
||
let module Server = Memory_interface.Server(Memory_server) in | ||
bind (); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we get an error if bind
fails to bind some of the expected calls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, but that's something that could be added to the library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of minor comments
Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>
Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>
Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>
Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Signed-off-by: Jon Ludlam jonathan.ludlam@citrix.com