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

CP-10203: XenAPI: api call for RDP on/off request #158

Merged
merged 1 commit into from Jan 6, 2015

Conversation

huizh
Copy link
Contributor

@huizh huizh commented Dec 11, 2014

Xapi delegates the xenstore operations to xenopsd.

Signed-off-by: Hui Zhang hui.zhang@citrix.com

@@ -69,6 +69,7 @@ module VM = struct
let s3suspend _ _ = unimplemented "VM.s3suspend"
let s3resume _ _ = unimplemented "VM.s3resume"
let get_state _ = Xenops_utils.halted_vm
let beg_rdp _ _ = ()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could return unimplemented like the s3suspend and s3resume. This is more accurate since returning unit is unclear whether it has done something or nothing.

@simonjbeaumont
Copy link
Collaborator

My guess is it's too late to change the name of this API call. To me, beg_rdp just sounds strange (almost unprofessional?). Anyway, it seems odd that this would take a boolean since begging for RDP has the connotation that you are asking for it to be enabled.

Apart from the aesthetics of this API design, it seems functionally sane.

@thomassa
Copy link
Contributor

You can blame me for the name.
I agree that it sounds a little odd, but my thinking was that it's short and it makes it clear that we are only asking the guest to turn RDP on/off without any assurance of whether our request will be granted.

@huizh
Copy link
Contributor Author

huizh commented Dec 15, 2014

Thanks Simon for the comment on lib/xenops_server_skeleton.ml.
I've made update.

About the API name, Simon and Thomas, could you please let me know your opinion? To change it to a more suitable name (any suggestion?) or just keep it?
Thanks.

@robhoes
Copy link
Member

robhoes commented Dec 15, 2014

Like @simonjbeaumont, I am not very comfortable with the name of the API call. This is even more important for the higher-level public API introduced in xapi-project/xen-api#2042. How about "request" rather than "beg"?

@huizh
Copy link
Contributor Author

huizh commented Dec 16, 2014

Thanks Rob for the suggestion.
Code has been updated accordingly.

@simonjbeaumont
Copy link
Collaborator

@huizh: sorry to mess you around, but did you see @djs55's comment here: xapi-project/xapi-project.github.io#12 (comment). He's suggesting a different API call name.

Signed-off-by: Hui Zhang <hui.zhang@citrix.com>
@huizh
Copy link
Contributor Author

huizh commented Dec 30, 2014

Thanks all for the comments.
There's discussion and decision made with xapi-project/xapi-project.github.io#12 (comment).
Based on that, we would keep the code.
Hope it's clear now.
Could you please correct me if there's any misunderstanding and review the code?

Much appreciate.

thomassa added a commit that referenced this pull request Jan 6, 2015
CP-10203: XenAPI: api call for RDP on/off request
@thomassa thomassa merged commit 9848704 into xapi-project:master Jan 6, 2015
@huizh
Copy link
Contributor Author

huizh commented Jan 7, 2015

Thanks @thomassa

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.

None yet

4 participants