-
Notifications
You must be signed in to change notification settings - Fork 2
Implement PVS_proxy module #7
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
Conversation
|
Needs xenserver/xcp-idl#5. |
| get () | ||
| in | ||
| get (); | ||
| Buffer.contents buf |
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.
This function looks OK, but how about some tests? 😉 Also the module could do with a .mli
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.
Can't we use a JSON library? The JSON syntax is not complicated but I don't have a lot of faith that this covers all cases. A good summary is here: http://json.org.
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.
We are using a JSON-RPC library to create the calls and parse the responses: ocaml-rpc. However, for the transport layer, ocaml-rpc only supports HTTP, while we agreed to use a "raw" stream socket for this. Ideally, we'd eventually upstream this into ocaml-rpc.
Signed-off-by: Rob Hoes <[email protected]>
Signed-off-by: Rob Hoes <[email protected]>
7420dcd to
cf268bd
Compare
Signed-off-by: Rob Hoes <[email protected]>
Signed-off-by: Rob Hoes <[email protected]>
cf268bd to
e40a491
Compare
|
@johnelse Could you re-review please? |
| let configure_farm _ dbg config = | ||
| debug "Configuring PVS proxy for farm %s" config.farm_uuid; | ||
| let call = {Rpc.name = "configure_farm"; params = [rpc_of_t config]} in | ||
| let _ = do_call call in |
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 need to handle any errors returned from the server here?
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.
Yes, but at this point, we don't know which errors the daemon may return yet. We will have to fill this in later.
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.
Ok, I think this looks good then.
No description provided.