After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 710173 - Make Vinagre accept IPv6 addresses
Make Vinagre accept IPv6 addresses
Status: RESOLVED WONTFIX
Product: vinagre
Classification: Applications
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: vinagre-maint
vinagre-maint
: 757797 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-10-15 09:01 UTC by tarnyko
Modified: 2015-11-09 12:42 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
vinagre_acceptipv6.patch (898 bytes, patch)
2013-10-15 09:01 UTC, tarnyko
none Details | Review
Accept IPv6 addresses (1.12 KB, patch)
2015-09-24 21:49 UTC, tarnyko
committed Details | Review

Description tarnyko 2013-10-15 09:01:31 UTC
Created attachment 257333 [details] [review]
vinagre_acceptipv6.patch

Currently, Vinagre does not accept direct typing of IPv6 addresses in the "host" field. You have to use an hostname (which can then eventually be linked to an IPv6 address).

A check seems to be wrong in the source code ; I added another check, too. Please consider the attached patch, which fixes this behavior.
Comment 1 tarnyko 2013-10-17 12:03:43 UTC
Also affects latest master.
Comment 2 David King 2015-09-22 13:37:46 UTC
Review of attachment 257333 [details] [review]:

Oops, sorry for missing this patch. One bug which needs fixing.

::: vinagre/vinagre-connection.c
@@ +533,3 @@
+  is_ipv6 = g_strstr_len (lhost, -1, ":");
+  if (is_ipv6)
+    lhost = g_strconcat (g_strdup ("["), g_strdup (lhost), g_strdup ("]"), NULL);

g_strconcat() duplicates the strings, so you don't need the g_strdup().
Comment 3 tarnyko 2015-09-24 21:49:49 UTC
Created attachment 312086 [details] [review]
Accept IPv6 addresses

True. Please find attached a new version of the patch, which :

- applies cleanly on latest master ;
- does not leak memory with unneeded g_strdup()s ;
- is shorter.
Comment 4 David King 2015-09-25 11:37:18 UTC
Review of attachment 312086 [details] [review]:

Thanks for the updated patch. Pushed to master as commit 1b2df81a539dbdb629466cd1f670545d1b4768d6.
Comment 5 Michał Kępień 2015-10-15 12:56:42 UTC
Please excuse my harshness and feel free to correct me if I'm mistaken, but this whole issue and the accepted patch seem to be horribly wrong.

This bug should be marked as INVALID the moment it was reported because Vinagre has already supported the requested feature since 2009 - see 7c2e351780598a77552b23b0ac678461fcd87634.

Anyway, the accepted patch makes it impossible to specify either a custom display number ("host:display") or a custom port number ("host::port") for the target - just look a dozen lines below the place that this patch inserts itself at. I believe a revert is in order.
Comment 6 tvwp5 2015-10-18 00:36:10 UTC
(In reply to Michał Kępień from comment #5)
> Please excuse my harshness and feel free to correct me if I'm mistaken, but
> this whole issue and the accepted patch seem to be horribly wrong.
> 
> This bug should be marked as INVALID the moment it was reported because
> Vinagre has already supported the requested feature since 2009 - see
> 7c2e351780598a77552b23b0ac678461fcd87634.
> 
> Anyway, the accepted patch makes it impossible to specify either a custom
> display number ("host:display") or a custom port number ("host::port") for
> the target - just look a dozen lines below the place that this patch inserts
> itself at. I believe a revert is in order.

I concur that this change should be reverted. This broke specifying custom
port numbers for me.
Comment 7 David King 2015-10-19 08:47:16 UTC
Reverted in master and gnome-3-18 branches.
Comment 8 David King 2015-11-09 12:42:01 UTC
*** Bug 757797 has been marked as a duplicate of this bug. ***