Re: [Spice-devel] [PATCH spice-gtk] channel-main: Convert text line-endings if necessary (rhbz#752350)
Hi
On Mon, Jun 24, 2013 at 2:31 PM, Hans de Goede hdego...@redhat.com wrote:
Signed-off-by: Hans de Goede hdego...@redhat.com
---
gtk/channel-main.c | 73
+++---
1 file changed, 69 insertions(+), 4 deletions(-)
diff --git a/gtk/channel-main.c b/gtk/channel-main.c
index b58af52..b9e0da2 100644
--- a/gtk/channel-main.c
+++ b/gtk/channel-main.c
@@ -1181,6 +1181,24 @@ static void agent_announce_caps(SpiceMainChannel
*channel)
#define HAS_CLIPBOARD_SELECTION(c) \
VD_AGENT_HAS_CAPABILITY((c)-agent_caps, G_N_ELEMENTS((c)-agent_caps),
VD_AGENT_CAP_CLIPBOARD_SELECTION)
+#define GUEST_LINEEND_LF(c) \
+VD_AGENT_HAS_CAPABILITY((c)-agent_caps, G_N_ELEMENTS((c)-agent_caps),
VD_AGENT_CAP_GUEST_LINEEND_LF)
+
+#define GUEST_LINEEND_CRLF(c) \
+VD_AGENT_HAS_CAPABILITY((c)-agent_caps, G_N_ELEMENTS((c)-agent_caps),
VD_AGENT_CAP_GUEST_LINEEND_CRLF)
+
+#ifdef G_OS_UNIX
+#define CLIENT_LINEEND_LF 1
+#else
+#define CLIENT_LINEEND_LF 0
+#endif
+
+#ifdef G_OS_WIN32
+#define CLIENT_LINEEND_CRLF 1
+#else
+#define CLIENT_LINEEND_CRLF 0
+#endif
+
/* any context: the message is not flushed immediately,
you can wakeup() the channel coroutine or send_msg_queue() */
static void agent_clipboard_grab(SpiceMainChannel *channel, guint selection,
@@ -1751,6 +1769,29 @@ static void file_xfer_handle_status(SpiceMainChannel
*channel,
file_xfer_completed(task, error);
}
+/* any context */
+static guchar *convert_lineend(const guchar *in, gsize *size,
+ const gchar *from, const gchar *to)
+{
+gchar *nul_terminated, **split, *out;
+
+/* Nul-terminate */
+nul_terminated = g_malloc(*size + 1);
+memcpy(nul_terminated, in, *size);
+nul_terminated[*size] = 0;
+
+/* Convert */
+split = g_strsplit(nul_terminated, from, -1);
+out = g_strjoinv(to, split);
I am not really fond of this implementation that uses up to 4x the
initial memory and may not be correct.
Instead, I would use g_data_input_stream_read_line (_utf8 if
available) from a memory input stream. and put that line by line in a
data output / memory output stream (changing ending with the one
appropriate).
That could be left for later, unless you feel like doing it now.
+*size = strlen(out);
+
+/* Clean-up */
+g_strfreev(split);
+g_free(nul_terminated);
+
+return (guchar *)out;
+}
+
/* coroutine context */
static void main_agent_handle_msg(SpiceChannel *channel,
VDAgentMessage *msg, gpointer payload)
@@ -1809,12 +1850,22 @@ static void main_agent_handle_msg(SpiceChannel
*channel,
case VD_AGENT_CLIPBOARD:
{
VDAgentClipboard *cb = payload;
+guchar *data = cb-data;
+gsize size = msg-size - sizeof(VDAgentClipboard);
+if (cb-type == VD_AGENT_CLIPBOARD_UTF8_TEXT) {
+if (GUEST_LINEEND_LF(c) CLIENT_LINEEND_CRLF)
+data = convert_lineend(data, size, \n, \r\n);
+if (GUEST_LINEEND_CRLF(c) CLIENT_LINEEND_LF)
+data = convert_lineend(data, size, \r\n, \n);
+}
emit_main_context(channel, SPICE_MAIN_CLIPBOARD_SELECTION, selection,
- cb-type, cb-data, msg-size -
sizeof(VDAgentClipboard));
+ cb-type, data, size);
- if (selection == VD_AGENT_CLIPBOARD_SELECTION_CLIPBOARD)
+if (selection == VD_AGENT_CLIPBOARD_SELECTION_CLIPBOARD)
emit_main_context(channel, SPICE_MAIN_CLIPBOARD,
- cb-type, cb-data, msg-size -
sizeof(VDAgentClipboard));
+ cb-type, data, size);
+if (data != cb-data)
+g_free(data);
break;
}
case VD_AGENT_CLIPBOARD_GRAB:
@@ -2554,13 +2605,27 @@ void spice_main_clipboard_notify(SpiceMainChannel
*channel,
* Since: 0.6
**/
void spice_main_clipboard_selection_notify(SpiceMainChannel *channel, guint
selection,
- guint32 type, const guchar *data,
size_t size)
+ guint32 type, const guchar
*_data, size_t _size)
{
+const guchar *data = _data;
why const here (since convert_lineend() returns non-const)?
+gsize size = _size;
+
g_return_if_fail(channel != NULL);
g_return_if_fail(SPICE_IS_MAIN_CHANNEL(channel));
+SpiceMainChannelPrivate *c = channel-priv;
+
+if (type == VD_AGENT_CLIPBOARD_UTF8_TEXT) {
+if (CLIENT_LINEEND_CRLF GUEST_LINEEND_LF(c))
+data = convert_lineend(data, size, \r\n, \n);
+if (CLIENT_LINEEND_LF GUEST_LINEEND_CRLF(c))
+data = convert_lineend(data, size, \n, \r\n);
+}
agent_clipboard_notify(channel, selection, type, data, size);
spice_channel_wakeup(SPICE_CHANNEL(channel), FALSE);
+
+if (data != _data)
+
Re: [Spice-devel] [PATCH spice-gtk] channel-main: Convert text line-endings if necessary (rhbz#752350)
On Mon, Jun 24, 2013 at 02:31:27PM +0200, Hans de Goede wrote: Signed-off-by: Hans de Goede hdego...@redhat.com --- gtk/channel-main.c | 73 +++--- 1 file changed, 69 insertions(+), 4 deletions(-) IIUC, implicit in your patch here is that the communication protocol between spice client guest agent uses guest OS specific line ending markers, and the spice client must do all conversions to guest OS format. Is that really what we want ? Personally I would have said that the line ending markers should be invariant (always \n) in the comms protocol and that each end point must handle conversion to/from \r\n if required. But perhaps there are backwards compat issues forcing your choice here ? Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk] channel-main: Convert text line-endings if necessary (rhbz#752350)
Hi, On 06/24/2013 02:39 PM, Daniel P. Berrange wrote: On Mon, Jun 24, 2013 at 02:31:27PM +0200, Hans de Goede wrote: Signed-off-by: Hans de Goede hdego...@redhat.com --- gtk/channel-main.c | 73 +++--- 1 file changed, 69 insertions(+), 4 deletions(-) IIUC, implicit in your patch here is that the communication protocol between spice client guest agent uses guest OS specific line ending markers, and the spice client must do all conversions to guest OS format. Is that really what we want ? Yes, this is by design. The reason for this is that there are no real conventions as to what line-endings to use inside the clipboard data, so ie the clipboard on unix (X) may contain text data with CRLF line-endings. And some programs may depend on this. So to minimize the change of breaking things we don't want to do any conversion when doing copy and paste between a unix guest and unix client, or a windows guest and windows client. Personally I would have said that the line ending markers should be invariant (always \n) in the comms protocol and that each end point must handle conversion to/from \r\n if required. That was my initial design too, but see above. But perhaps there are backwards compat issues forcing your choice here ? Well currently we don't do any conversions, so keeping the wire format in guest encoding makes things a bit easier wrt backward compat handling, but we could have moved to always use \n on the wire using guest capabilities bits, so that is not the main reason for this. Regards, Hans ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk] channel-main: Convert text line-endings if necessary (rhbz#752350)
On Mon, Jun 24, 2013 at 02:45:25PM +0200, Hans de Goede wrote: Hi, On 06/24/2013 02:39 PM, Daniel P. Berrange wrote: On Mon, Jun 24, 2013 at 02:31:27PM +0200, Hans de Goede wrote: Signed-off-by: Hans de Goede hdego...@redhat.com --- gtk/channel-main.c | 73 +++--- 1 file changed, 69 insertions(+), 4 deletions(-) IIUC, implicit in your patch here is that the communication protocol between spice client guest agent uses guest OS specific line ending markers, and the spice client must do all conversions to guest OS format. Is that really what we want ? Yes, this is by design. The reason for this is that there are no real conventions as to what line-endings to use inside the clipboard data, so ie the clipboard on unix (X) may contain text data with CRLF line-endings. And some programs may depend on this. So to minimize the change of breaking things we don't want to do any conversion when doing copy and paste between a unix guest and unix client, or a windows guest and windows client. Personally I would have said that the line ending markers should be invariant (always \n) in the comms protocol and that each end point must handle conversion to/from \r\n if required. That was my initial design too, but see above. But perhaps there are backwards compat issues forcing your choice here ? Well currently we don't do any conversions, so keeping the wire format in guest encoding makes things a bit easier wrt backward compat handling, but we could have moved to always use \n on the wire using guest capabilities bits, so that is not the main reason for this. Ok, that makes sense. Would be nice to describe this rationale in the commit message for benefit of people looking back at this change years down the road. Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk] channel-main: Convert text line-endings if necessary (rhbz#752350)
Hi, On 06/24/2013 03:04 PM, Daniel P. Berrange wrote: On Mon, Jun 24, 2013 at 02:45:25PM +0200, Hans de Goede wrote: Hi, On 06/24/2013 02:39 PM, Daniel P. Berrange wrote: On Mon, Jun 24, 2013 at 02:31:27PM +0200, Hans de Goede wrote: Signed-off-by: Hans de Goede hdego...@redhat.com --- gtk/channel-main.c | 73 +++--- 1 file changed, 69 insertions(+), 4 deletions(-) IIUC, implicit in your patch here is that the communication protocol between spice client guest agent uses guest OS specific line ending markers, and the spice client must do all conversions to guest OS format. Is that really what we want ? Yes, this is by design. The reason for this is that there are no real conventions as to what line-endings to use inside the clipboard data, so ie the clipboard on unix (X) may contain text data with CRLF line-endings. And some programs may depend on this. So to minimize the change of breaking things we don't want to do any conversion when doing copy and paste between a unix guest and unix client, or a windows guest and windows client. Personally I would have said that the line ending markers should be invariant (always \n) in the comms protocol and that each end point must handle conversion to/from \r\n if required. That was my initial design too, but see above. But perhaps there are backwards compat issues forcing your choice here ? Well currently we don't do any conversions, so keeping the wire format in guest encoding makes things a bit easier wrt backward compat handling, but we could have moved to always use \n on the wire using guest capabilities bits, so that is not the main reason for this. Ok, that makes sense. Would be nice to describe this rationale in the commit message for benefit of people looking back at this change years down the road. It is already described in the commit message of the protocol update for this, which is IMHO the proper place for this: http://cgit.freedesktop.org/spice/spice-protocol/commit/?id=7be0e88e7e03a956b364cc847aad11b96ed47273 Regards, Hans ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk] channel-main: Convert text line-endings if necessary (rhbz#752350)
On Mon, Jun 24, 2013 at 03:23:30PM +0200, Hans de Goede wrote: Hi, On 06/24/2013 03:04 PM, Daniel P. Berrange wrote: On Mon, Jun 24, 2013 at 02:45:25PM +0200, Hans de Goede wrote: Hi, On 06/24/2013 02:39 PM, Daniel P. Berrange wrote: On Mon, Jun 24, 2013 at 02:31:27PM +0200, Hans de Goede wrote: Signed-off-by: Hans de Goede hdego...@redhat.com --- gtk/channel-main.c | 73 +++--- 1 file changed, 69 insertions(+), 4 deletions(-) IIUC, implicit in your patch here is that the communication protocol between spice client guest agent uses guest OS specific line ending markers, and the spice client must do all conversions to guest OS format. Is that really what we want ? Yes, this is by design. The reason for this is that there are no real conventions as to what line-endings to use inside the clipboard data, so ie the clipboard on unix (X) may contain text data with CRLF line-endings. And some programs may depend on this. So to minimize the change of breaking things we don't want to do any conversion when doing copy and paste between a unix guest and unix client, or a windows guest and windows client. Personally I would have said that the line ending markers should be invariant (always \n) in the comms protocol and that each end point must handle conversion to/from \r\n if required. That was my initial design too, but see above. But perhaps there are backwards compat issues forcing your choice here ? Well currently we don't do any conversions, so keeping the wire format in guest encoding makes things a bit easier wrt backward compat handling, but we could have moved to always use \n on the wire using guest capabilities bits, so that is not the main reason for this. Ok, that makes sense. Would be nice to describe this rationale in the commit message for benefit of people looking back at this change years down the road. It is already described in the commit message of the protocol update for this, which is IMHO the proper place for this: http://cgit.freedesktop.org/spice/spice-protocol/commit/?id=7be0e88e7e03a956b364cc847aad11b96ed47273 IMHO it should still be described here, or that commit could be referenced here as the source of the policy being implemented. Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
