GNOME Bugzilla – Bug 743303
Add spice+unix support
Last modified: 2016-06-28 14:42:08 UTC
Spice-gtk will allow connection to UNIX socket server, the patches are under review for spice-gtk atm. Let's teach Boxes to deal with spice+unix:// URI.
Created attachment 295111 [details] [review] WIP: add spice+unix support
(In reply to Marc-Andre Lureau from comment #1) > Created attachment 295111 [details] [review] [review] > WIP: add spice+unix support Doesn't bug#738573 make this obsolete?
(In reply to Zeeshan Ali (Khattak) from comment #2) > (In reply to Marc-Andre Lureau from comment #1) > > Created attachment 295111 [details] [review] [review] [review] > > WIP: add spice+unix support > > Doesn't bug#738573 make this obsolete? not fully, Boxes should still allow you to add "remote" VMs with spice+unix (VM not managed by libvirt in this case) please apply
(In reply to Marc-Andre Lureau from comment #3) > (In reply to Zeeshan Ali (Khattak) from comment #2) > > (In reply to Marc-Andre Lureau from comment #1) > > > Created attachment 295111 [details] [review] [review] [review] [review] > > > WIP: add spice+unix support > > > > Doesn't bug#738573 make this obsolete? > > not fully, Boxes should still allow you to add "remote" VMs with spice+unix > (VM not managed by libvirt in this case) Ah ok. > please apply You want me to apply a patch author marked as 'WIP'? :) I'll look into it soon..
I tested the patch (it applies cleanly), it allows to connect to vms created by qemu, eg: qemu-kvm -spice disable-ticketing,unix,addr=/tmp/spice.sock
(In reply to Pavel Grunt from comment #5) > I tested the patch (it applies cleanly), it allows to connect to vms created > by qemu, eg: qemu-kvm -spice disable-ticketing,unix,addr=/tmp/spice.sock Cool, thanks. Could you please also update the patch?
(In reply to Zeeshan Ali (Khattak) from comment #6) > (In reply to Pavel Grunt from comment #5) > > I tested the patch (it applies cleanly), it allows to connect to vms created > > by qemu, eg: qemu-kvm -spice disable-ticketing,unix,addr=/tmp/spice.sock > > Cool, thanks. Could you please also update the patch? At least commit log needs updating and we gotta remove the "WIP" tag. :)
Created attachment 326702 [details] [review] spice-display: handle SPICE_CHANNEL_OPENED event Do not log a debug message, the event is expected. Splitted from the original patch
Created attachment 326704 [details] [review] Add spice+unix support changes since WIP: * channel event handling moved to separated patch * added check in spice_validate_uri(), otherwise an uri like "spice+anything://host:port" would be valid
(In reply to Pavel Grunt from comment #9) > Created attachment 326704 [details] [review] [review] sorry, it should obsolete the original patch, not the patch from the comment 8
Review of attachment 326702 [details] [review]: ack
Review of attachment 326704 [details] [review]: good otherwise. ::: src/spice-display.vala @@ +558,3 @@ + (uri.query_raw ?? uri.query) != null) + throw new Boxes.Error.INVALID (_("Invalid Spice UNIX URI")); + } else if (uri.scheme.has_prefix("spice+")) { Don't think you need to explicitly check for this specific case. How about this: if (uri.scheme == "spice+unix") { ... } else if (port <= 0 && tls_port <= 0) { ... } else { throw new Boxes.Error.INVALID (_("Invalid URI")); } ::: src/wizard.vala @@ +326,2 @@ spice_validate_uri (uri_as_text); + source.source_type = "spice"; i'm guessing this is needed now because source_type is assumed to be same as URI scheme if not explicitly set?
Review of attachment 326704 [details] [review]: ::: src/spice-display.vala @@ +558,3 @@ + (uri.query_raw ?? uri.query) != null) + throw new Boxes.Error.INVALID (_("Invalid Spice UNIX URI")); + } else if (uri.scheme.has_prefix("spice+")) { you need to check for invalid scheme before checking invalid port. The way you suggested "spice+anything://host:port" would be valid ::: src/wizard.vala @@ +326,2 @@ spice_validate_uri (uri_as_text); + source.source_type = "spice"; So it would be "spice+unix", which is not valid, no?
Review of attachment 326704 [details] [review]: ::: src/spice-display.vala @@ +558,3 @@ + (uri.query_raw ?? uri.query) != null) + throw new Boxes.Error.INVALID (_("Invalid Spice UNIX URI")); + } else if (uri.scheme.has_prefix("spice+")) { true. I think you want to check for all invalid URIs then, not just for some specific kind: switch (uri.scheme) { case "spice+unix": .. break; case "spice:" if (port <= 0 && tls_port <= 0) { ... } break; default: throw new Boxes.Error.INVALID (_("Invalid URI")); break; } ::: src/wizard.vala @@ +326,2 @@ spice_validate_uri (uri_as_text); + source.source_type = "spice"; yes.
Created attachment 326780 [details] [review] spice-display: Be more strict when validating URI Throw an error if the URI scheme is invalid.
Created attachment 326781 [details] [review] Add spice+unix support Spice-gtk allows to connect to a UNIX socket server since v0.28. Let's teach Boxes to deal with spice+unix:// URI. changes: "if" to "case" due to the new patch "spice-display: Be more strict when validating URI"
Review of attachment 326780 [details] [review]: Good call separateing out this change. ::: src/spice-display.vala @@ +557,3 @@ + case "spice": + if (port <= 0 && tls_port <= 0) + throw new Boxes.Error.INVALID (_("Missing port in Spice URI")); We might take this opportunity and fix this translated message a bit: "Missing port in SPICE URL" since SPICE is correct way of putting it and we used the terms SPICE and URL in the UI. But i guess you'll want to put that in a seperate patch that goes before this one. @@ +560,3 @@ + break; + default: + throw new Boxes.Error.INVALID (_("Invalid URI")); URL in here too.
Review of attachment 326781 [details] [review]: Nitpick on description part of commit log: Each line is supposed to be 74 characters, breaking it before that limit exceeds makes it look like you are starting a new para but without empty line between two paras. ::: src/spice-display.vala @@ +561,3 @@ + case "spice+unix": + if (port > 0 || + (uri.query_raw ?? uri.query) != null) Use of ?? operator here only makes it less readable. if (port > 0 || uri.query_raw != null || uri.query != null) would be more readable IMO. BTW, in Boxes code, the lines are 120 columns so no need to break the condition into multiple lines here. @@ +562,3 @@ + if (port > 0 || + (uri.query_raw ?? uri.query) != null) + throw new Boxes.Error.INVALID (_("Invalid Spice UNIX URI")); Don't think we need to tell user that it's a UNIX URL, let's just re-use the generic error here. ::: src/wizard.vala @@ +323,3 @@ source = new CollectionSource (uri.server ?? uri_as_text, uri.scheme, uri_as_text); + if (uri.scheme == "spice" || uri.scheme.has_prefix("spice+")) { why check of prefix and not just "spice+unix" scheme?
Review of attachment 326780 [details] [review]: ::: src/spice-display.vala @@ +557,3 @@ + case "spice": + if (port <= 0 && tls_port <= 0) + throw new Boxes.Error.INVALID (_("Missing port in Spice URI")); I would say it is a different bug - I guess you'll want to change all the strings (grep shows that URI is used in strings in app.vala, libvirt-machine-properties.vala, ovirt-machine.vala, vnc-display.vala...)
Review of attachment 326780 [details] [review]: ::: src/spice-display.vala @@ +557,3 @@ + case "spice": + if (port <= 0 && tls_port <= 0) + throw new Boxes.Error.INVALID (_("Missing port in Spice URI")); Ah ok, I did not realize but I don't think this small change justifies creating another bug.
Created attachment 326794 [details] [review] Add spice+unix support changes: line break around 74 characters in the commit message removed usage of the '??' operator reused the generic error message explicit check for the scheme instead of prefix
Ouch, I pushed these patches by mistake while pushing the patch for bug#723008. :( Pavel, sorry you'll have to rebase your patches now. :( Attachment 326702 [details] pushed as 62f211b - spice-display: handle SPICE_CHANNEL_OPENED event Attachment 326794 [details] pushed as e1fa2e0 - Add spice+unix support
(In reply to Zeeshan Ali (Khattak) from comment #22) > Ouch, I pushed these patches by mistake while pushing the patch for > bug#723008. :( Err. I'm on a roll today. I meant bug#763772.
Created attachment 326944 [details] [review] spice-display: Be more strict when validating URI rebased, checking for the prefix of the scheme in wizard is enough as the full scheme is validated in spice_validate_uri()
I mean, could you please fix all the issue pointed out in all patches, including the committed ones? For committed patches, you'd obviously need to fix the issues in new patches. Sorry for the mess again.
(In reply to Zeeshan Ali (Khattak) from comment #25) > I mean, could you please fix all the issue pointed out in all patches, > including the committed ones? For committed patches, you'd obviously need to > fix the issues in new patches. Sorry for the mess again. Sorry, I don't know what is not fixed by the patch in the comment 24.
Review of attachment 326944 [details] [review]: ::: src/spice-display.vala @@ +557,3 @@ + case "spice+unix": + if (port > 0 || uri.query_raw != null || uri.query != null) + throw new Boxes.Error.INVALID (_("Invalid URI")); As we discussed, this should be "URL", not "URI". You informed me that "URI" is already being used in these translated messages so I asked if you could kindly change them (in a separate change) first.
(In reply to Zeeshan Ali (Khattak) from comment #27) > Review of attachment 326944 [details] [review] [review]: > > ::: src/spice-display.vala > @@ +557,3 @@ > + case "spice+unix": > + if (port > 0 || uri.query_raw != null || uri.query != null) > + throw new Boxes.Error.INVALID (_("Invalid URI")); > > As we discussed, this should be "URL", not "URI". You informed me that "URI" > is already being used in these translated messages so I asked if you could > kindly change them (in a separate change) first. We did not discuss about it, I don't know why it is wrong. Actually in comment 20 you were ok with my suggestion that it is a different bug - if you do not want to open bug for it, I can do it for you :) Currently URI is used everywhere, URL is used in 2 strings. I don't know why now you don't like URI. gtk has function gtk_show_uri, wizard has method open_with_uri, many variables have uri in their name. In my opinion this mass rename (~140 occurrences of uri) does not fit to this bug...
(In reply to Pavel Grunt from comment #28) > (In reply to Zeeshan Ali (Khattak) from comment #27) > > Review of attachment 326944 [details] [review] [review] [review]: > > > > ::: src/spice-display.vala > > @@ +557,3 @@ > > + case "spice+unix": > > + if (port > 0 || uri.query_raw != null || uri.query != null) > > + throw new Boxes.Error.INVALID (_("Invalid URI")); > > > > As we discussed, this should be "URL", not "URI". You informed me that "URI" > > is already being used in these translated messages so I asked if you could > > kindly change them (in a separate change) first. > > We did not discuss about it, I don't know why it is wrong. Actually in > comment 20 you were ok with my suggestion that it is a different bug - if > you do not want to open bug for it, I can do it for you :) No, I said the change doesn't justify a bug but if you prefer another bug, please do create another one and put the patch in there. My point was that just because some existing strings are the wrong way, doesn't mean we should add to that but rather fix those strings first. > Currently URI is used everywhere, URL is used in 2 strings. I don't know why > now you don't like URI. gtk has function gtk_show_uri, wizard has method > open_with_uri, many variables have uri in their name. In my opinion this > mass rename (~140 occurrences of uri) does not fit to this bug... There is a reason why I included the term "translated". We can call it URI or uri in the code and that is all fine, I was talking of user-visible strings. See the URL entry page of wizard and you'll see we call it URL in labels so I want to be consistent with that: User is asked to enter a URL and then if it's incorrect URL, they get an error talking about URI.
Created attachment 327757 [details] [review] spice-display: Be more strict when validating URI Throw an error if the URI scheme is invalid.
Created attachment 327758 [details] [review] Use URL in strings instead of URI
Review of attachment 327757 [details] [review]: Sorry to come to this review so late but it probably needs a rebase now, cause git(-bz) dies on this: Applying: spice-display: Be more strict when validating URI fatal: sha1 information is lacking or useless (src/spice-display.vala). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 spice-display: Be more strict when validating URI The copy of the patch that failed is found in: /home/zeenix/checkout/gnome/gnome-boxes/.git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/spice-display-Be-more-strict-when-validating-URI-sB__kW.patch
Created attachment 328134 [details] [review] Use URL in strings instead of URI
Created attachment 328135 [details] [review] spice-display: Be more strict when validating URI Throw an error if the URI scheme is invalid.
Review of attachment 328134 [details] [review]: A bit of detail in commit log would be nice.
Review of attachment 328135 [details] [review]: The commit log (the you re-used from an already commit patch) is not correct summary of this change. Please update.
Review of attachment 328135 [details] [review]: Please tell from which commit I reused it. Anyway it is correct, currently it is not strict - it is allowed to use spice+unix://socket:port. This patch avoids it by strictly checking for the uri scheme. Without this patch no error is emitted, this patch changes it, so the error is emitted.
Review of attachment 328135 [details] [review]: Ah, my mess-up was even messier than I thought. I was talking of this patch: https://bugzilla.gnome.org/attachment.cgi?id=326780&action=diff which it turns out, was not the one that I applied (accidently) but the earlier patch "Add spice+unix support", which contained these changes. :(
Attachment 328134 [details] pushed as a01f426 - Use URL in strings instead of URI Attachment 328135 [details] pushed as e04de70 - spice-display: Be more strict when validating URI