[spice] reworked to avoid locking requirements on the input channel

POSIX `send` is thread safe, to take advantage of this the code has been
changed to construct a contiguous buffer and perform the send in a
single operation preventing any risk of a race condition.

Only the main channel still requires an interlock as the VD agent
requires multiple sends to transmit a full buffer.
This commit is contained in:
Geoffrey McRae 2020-01-29 18:52:11 +11:00
parent 29f221d547
commit cc2c49644d
2 changed files with 111 additions and 110 deletions

View File

@ -1 +1 @@
B1-138-g2e32ceb6e0+1 B1-140-g4c185799f6+1

View File

@ -27,6 +27,7 @@ Place, Suite 330, Boston, MA 02111-1307 USA
#include <stdio.h> #include <stdio.h>
#include <stdint.h> #include <stdint.h>
#include <stdlib.h> #include <stdlib.h>
#include <stdatomic.h>
#include <sys/types.h> #include <sys/types.h>
#include <sys/socket.h> #include <sys/socket.h>
@ -65,6 +66,39 @@ Place, Suite 330, Boston, MA 02111-1307 USA
// possible number // possible number
#define SPICE_AGENT_TOKENS_MAX ~0 #define SPICE_AGENT_TOKENS_MAX ~0
#define SPICE_PACKET(htype, payloadType, extraData) \
({ \
uint8_t packet[sizeof(ssize_t) + sizeof(SpiceMiniDataHeader) + sizeof(payloadType)]; \
ssize_t * sz = (ssize_t*)packet; \
SpiceMiniDataHeader * header = (SpiceMiniDataHeader *)(packet + sizeof(ssize_t)); \
*sz = sizeof(SpiceMiniDataHeader) + sizeof(payloadType); \
header->type = (htype); \
header->size = sizeof(payloadType) + extraData; \
(payloadType *)(header + 1); \
})
/* only the main channel requires locking due to the agent needing to send
* multiple packets, all other channels are atomic */
#define SPICE_SEND_PACKET_LOCKED(channel, packet) \
({ \
SpiceMiniDataHeader * header = (SpiceMiniDataHeader *)(((uint8_t *)packet) - \
sizeof(SpiceMiniDataHeader)); \
ssize_t *sz = (ssize_t *)(((uint8_t *)header) - sizeof(ssize_t)); \
LG_LOCK((channel)->lock); \
const ssize_t wrote = spice_write_nl((channel), header, *sz); \
LG_UNLOCK((channel)->lock); \
wrote == *sz; \
})
#define SPICE_SEND_PACKET(channel, packet) \
({ \
SpiceMiniDataHeader * header = (SpiceMiniDataHeader *)(((uint8_t *)packet) - \
sizeof(SpiceMiniDataHeader)); \
ssize_t *sz = (ssize_t *)(((uint8_t *)header) - sizeof(ssize_t)); \
const ssize_t wrote = spice_write_nl((channel), header, *sz); \
wrote == *sz; \
})
// ============================================================================ // ============================================================================
// internal structures // internal structures
@ -77,7 +111,6 @@ struct SpiceChannel
int socket; int socket;
uint32_t ackFrequency; uint32_t ackFrequency;
uint32_t ackCount; uint32_t ackCount;
uint32_t serial;
LG_Lock lock; LG_Lock lock;
}; };
@ -110,7 +143,6 @@ struct Spice
bool hasAgent; bool hasAgent;
uint32_t serverTokens; uint32_t serverTokens;
uint32_t clientTokens;
uint32_t sessionID; uint32_t sessionID;
uint32_t channelID; uint32_t channelID;
struct SpiceChannel scMain; struct SpiceChannel scMain;
@ -165,14 +197,12 @@ static uint32_t spice_type_to_agent_type(SpiceDataType type);
static SpiceDataType agent_type_to_spice_type(uint32_t type); static SpiceDataType agent_type_to_spice_type(uint32_t type);
// thread safe read/write methods // thread safe read/write methods
bool spice_write_msg (struct SpiceChannel * channel, uint32_t type, const void * buffer, const ssize_t size);
bool spice_agent_write_msg (uint32_t type, const void * buffer, ssize_t size); bool spice_agent_write_msg (uint32_t type, const void * buffer, ssize_t size);
// non thread safe read/write methods (nl = non-locking) // non thread safe read/write methods (nl = non-locking)
bool spice_read_nl (const struct SpiceChannel * channel, void * buffer, const ssize_t size); bool spice_read_nl (const struct SpiceChannel * channel, void * buffer, const ssize_t size);
ssize_t spice_write_nl (const struct SpiceChannel * channel, const void * buffer, const ssize_t size); ssize_t spice_write_nl (const struct SpiceChannel * channel, const void * buffer, const ssize_t size);
bool spice_discard_nl (const struct SpiceChannel * channel, ssize_t size); bool spice_discard_nl(const struct SpiceChannel * channel, ssize_t size);
bool spice_write_msg_nl( struct SpiceChannel * channel, uint32_t type, const void * buffer, const ssize_t size, const ssize_t extra);
// ============================================================================ // ============================================================================
@ -307,7 +337,10 @@ bool spice_process_ack(struct SpiceChannel * channel)
return true; return true;
channel->ackCount = 0; channel->ackCount = 0;
return spice_write_msg(channel, SPICE_MSGC_ACK, "\0", 1);
char * ack = SPICE_PACKET(SPICE_MSGC_ACK, char, 0);
*ack = 0;
return SPICE_SEND_PACKET(channel, ack);
} }
// ============================================================================ // ============================================================================
@ -352,12 +385,11 @@ bool spice_on_common_read(struct SpiceChannel * channel, SpiceMiniDataHeader * h
channel->ackFrequency = in.window; channel->ackFrequency = in.window;
SpiceMsgcAckSync out; SpiceMsgcAckSync * out =
out.generation = in.generation; SPICE_PACKET(SPICE_MSGC_ACK_SYNC, SpiceMsgcAckSync, 0);
if (!spice_write_msg(channel, SPICE_MSGC_ACK_SYNC, &out, sizeof(out)))
return false;
return true; out->generation = in.generation;
return SPICE_SEND_PACKET_LOCKED(channel, out);
} }
case SPICE_MSG_PING: case SPICE_MSG_PING:
@ -378,13 +410,12 @@ bool spice_on_common_read(struct SpiceChannel * channel, SpiceMiniDataHeader * h
else else
DEBUG_PROTO("discard %d", discard); DEBUG_PROTO("discard %d", discard);
SpiceMsgcPong out; SpiceMsgcPong * out =
out.id = in.id; SPICE_PACKET(SPICE_MSGC_PONG, SpiceMsgcPong, 0);
out.timestamp = in.timestamp;
if (!spice_write_msg(channel, SPICE_MSGC_PONG, &out, sizeof(out)))
return false;
return true; out->id = in.id;
out->timestamp = in.timestamp;
return SPICE_SEND_PACKET_LOCKED(channel, out);
} }
case SPICE_MSG_WAIT_FOR_CHANNELS: case SPICE_MSG_WAIT_FOR_CHANNELS:
@ -455,7 +486,7 @@ bool spice_on_main_channel_read()
spice.sessionID = msg.session_id; spice.sessionID = msg.session_id;
spice.serverTokens = msg.agent_tokens; spice.serverTokens = msg.agent_tokens;
spice.hasAgent = msg.agent_connected; spice.hasAgent = msg.agent_connected;
if (spice.hasAgent && !spice_agent_connect()) if (spice.hasAgent && !spice_agent_connect())
{ {
spice_disconnect(); spice_disconnect();
@ -469,7 +500,8 @@ bool spice_on_main_channel_read()
return false; return false;
} }
if (!spice_write_msg(channel, SPICE_MSGC_MAIN_ATTACH_CHANNELS, NULL, 0)) void * packet = SPICE_PACKET(SPICE_MSGC_MAIN_ATTACH_CHANNELS, void, 0);
if (!SPICE_SEND_PACKET_LOCKED(channel, packet))
{ {
spice_disconnect(); spice_disconnect();
DEBUG_ERROR("failed to ask for channel list"); DEBUG_ERROR("failed to ask for channel list");
@ -703,7 +735,6 @@ bool spice_connect_channel(struct SpiceChannel * channel)
channel->initDone = false; channel->initDone = false;
channel->ackFrequency = 0; channel->ackFrequency = 0;
channel->ackCount = 0; channel->ackCount = 0;
channel->serial = 0;
LG_LOCK_INIT(channel->lock); LG_LOCK_INIT(channel->lock);
@ -908,8 +939,9 @@ bool spice_agent_connect()
{ {
DEBUG_INFO("Spice agent available, sending start"); DEBUG_INFO("Spice agent available, sending start");
spice.clientTokens = SPICE_AGENT_TOKENS_MAX; uint32_t * packet = SPICE_PACKET(SPICE_MSGC_MAIN_AGENT_START, uint32_t, 0);
if (!spice_write_msg(&spice.scMain, SPICE_MSGC_MAIN_AGENT_START, &spice.clientTokens, sizeof(spice.clientTokens))) *packet = SPICE_AGENT_TOKENS_MAX;
if (!SPICE_SEND_PACKET_LOCKED(&spice.scMain, packet))
{ {
DEBUG_ERROR("failed to send agent start message"); DEBUG_ERROR("failed to send agent start message");
return false; return false;
@ -1161,17 +1193,21 @@ bool spice_agent_send_caps(bool request)
bool spice_agent_write_msg(uint32_t type, const void * buffer, ssize_t size) bool spice_agent_write_msg(uint32_t type, const void * buffer, ssize_t size)
{ {
VDAgentMessage msg; uint8_t * buf = (uint8_t *)buffer;
msg.protocol = VD_AGENT_PROTOCOL; ssize_t toWrite = size > VD_AGENT_MAX_DATA_SIZE - sizeof(VDAgentMessage) ?
msg.type = type; VD_AGENT_MAX_DATA_SIZE - sizeof(VDAgentMessage) : size;
msg.opaque = 0;
msg.size = size; VDAgentMessage * msg =
SPICE_PACKET(SPICE_MSGC_MAIN_AGENT_DATA, VDAgentMessage, toWrite);
msg->protocol = VD_AGENT_PROTOCOL;
msg->type = type;
msg->opaque = 0;
msg->size = size;
LG_LOCK(spice.scMain.lock); LG_LOCK(spice.scMain.lock);
uint8_t * buf = (uint8_t *)buffer; if (!SPICE_SEND_PACKET(&spice.scMain, msg))
ssize_t toWrite = size > VD_AGENT_MAX_DATA_SIZE - sizeof(msg) ? VD_AGENT_MAX_DATA_SIZE - sizeof(msg) : size;
if (!spice_write_msg_nl(&spice.scMain, SPICE_MSGC_MAIN_AGENT_DATA, &msg, sizeof(msg), toWrite))
{ {
LG_UNLOCK(spice.scMain.lock); LG_UNLOCK(spice.scMain.lock);
DEBUG_ERROR("failed to write agent data header"); DEBUG_ERROR("failed to write agent data header");
@ -1189,7 +1225,8 @@ bool spice_agent_write_msg(uint32_t type, const void * buffer, ssize_t size)
} }
else else
{ {
ok = spice_write_msg_nl(&spice.scMain, SPICE_MSGC_MAIN_AGENT_DATA, buf, toWrite, 0); void * cont = SPICE_PACKET(SPICE_MSGC_MAIN_AGENT_DATA, void, toWrite);
ok = SPICE_SEND_PACKET(&spice.scMain, cont);
} }
if (!ok) if (!ok)
@ -1233,52 +1270,6 @@ ssize_t spice_write_nl(const struct SpiceChannel * channel, const void * buffer,
// ============================================================================ // ============================================================================
inline bool spice_write_msg(struct SpiceChannel * channel, uint32_t type, const void * buffer, const ssize_t size)
{
bool result;
LG_LOCK(channel->lock);
result = spice_write_msg_nl(channel, type, buffer, size, 0);
LG_UNLOCK(channel->lock);
return result;
}
// ============================================================================
bool spice_write_msg_nl(struct SpiceChannel * channel, uint32_t type, const void * buffer, const ssize_t size, const ssize_t extra)
{
if (!channel->ready)
{
DEBUG_ERROR("channel not ready");
return false;
}
SpiceMiniDataHeader header;
++channel->serial;
header.type = type;
header.size = size + extra;
if (spice_write_nl(channel, &header, sizeof(header)) != sizeof(header))
{
DEBUG_ERROR("failed to write message header");
return false;
}
if (buffer && size)
{
if (spice_write_nl(channel, buffer, size) != size)
{
DEBUG_ERROR("failed to write message body");
return false;
}
}
return true;
}
// ============================================================================
bool spice_read_nl(const struct SpiceChannel * channel, void * buffer, const ssize_t size) bool spice_read_nl(const struct SpiceChannel * channel, void * buffer, const ssize_t size)
{ {
if (!channel->connected) if (!channel->connected)
@ -1348,10 +1339,10 @@ bool spice_key_down(uint32_t code)
if (code > 0x100) if (code > 0x100)
code = 0xe0 | ((code - 0x100) << 8); code = 0xe0 | ((code - 0x100) << 8);
SpiceMsgcKeyDown msg; SpiceMsgcKeyDown * msg =
msg.code = code; SPICE_PACKET(SPICE_MSGC_INPUTS_KEY_DOWN, SpiceMsgcKeyDown, 0);
msg->code = code;
return spice_write_msg(&spice.scInputs, SPICE_MSGC_INPUTS_KEY_DOWN, &msg, sizeof(msg)); return SPICE_SEND_PACKET(&spice.scInputs, msg);
} }
// ============================================================================ // ============================================================================
@ -1370,10 +1361,10 @@ bool spice_key_up(uint32_t code)
else else
code = 0x80e0 | ((code - 0x100) << 8); code = 0x80e0 | ((code - 0x100) << 8);
SpiceMsgcKeyDown msg; SpiceMsgcKeyUp * msg =
msg.code = code; SPICE_PACKET(SPICE_MSGC_INPUTS_KEY_UP, SpiceMsgcKeyUp, 0);
msg->code = code;
return spice_write_msg(&spice.scInputs, SPICE_MSGC_INPUTS_KEY_UP, &msg, sizeof(msg)); return SPICE_SEND_PACKET(&spice.scInputs, msg);
} }
// ============================================================================ // ============================================================================
@ -1387,10 +1378,12 @@ bool spice_mouse_mode(bool server)
return false; return false;
} }
SpiceMsgcMainMouseModeRequest msg; SpiceMsgcMainMouseModeRequest * msg = SPICE_PACKET(
msg.mouse_mode = server ? SPICE_MOUSE_MODE_SERVER : SPICE_MOUSE_MODE_CLIENT; SPICE_MSGC_MAIN_MOUSE_MODE_REQUEST,
SpiceMsgcMainMouseModeRequest, 0);
return spice_write_msg(&spice.scMain, SPICE_MSGC_MAIN_MOUSE_MODE_REQUEST, &msg, sizeof(msg)); msg->mouse_mode = server ? SPICE_MOUSE_MODE_SERVER : SPICE_MOUSE_MODE_CLIENT;
return SPICE_SEND_PACKET_LOCKED(&spice.scMain, msg);
} }
// ============================================================================ // ============================================================================
@ -1404,14 +1397,16 @@ bool spice_mouse_position(uint32_t x, uint32_t y)
return false; return false;
} }
SpiceMsgcMousePosition msg; SpiceMsgcMousePosition * msg =
msg.x = x; SPICE_PACKET(SPICE_MSGC_INPUTS_MOUSE_POSITION, SpiceMsgcMousePosition, 0);
msg.y = y;
msg.button_state = spice.mouse.buttonState; msg->x = x;
msg.display_id = 0; msg->y = y;
msg->button_state = spice.mouse.buttonState;
msg->display_id = 0;
__sync_fetch_and_add(&spice.mouse.sentCount, 1); __sync_fetch_and_add(&spice.mouse.sentCount, 1);
return spice_write_msg(&spice.scInputs, SPICE_MSGC_INPUTS_MOUSE_POSITION, &msg, sizeof(msg)); return SPICE_SEND_PACKET(&spice.scInputs, msg);
} }
// ============================================================================ // ============================================================================
@ -1425,13 +1420,15 @@ bool spice_mouse_motion(int32_t x, int32_t y)
return false; return false;
} }
SpiceMsgcMouseMotion msg; SpiceMsgcMouseMotion * msg =
msg.x = x; SPICE_PACKET(SPICE_MSGC_INPUTS_MOUSE_MOTION, SpiceMsgcMouseMotion, 0);
msg.y = y;
msg.button_state = spice.mouse.buttonState; msg->x = x;
msg->y = y;
msg->button_state = spice.mouse.buttonState;
__sync_fetch_and_add(&spice.mouse.sentCount, 1); __sync_fetch_and_add(&spice.mouse.sentCount, 1);
return spice_write_msg(&spice.scInputs, SPICE_MSGC_INPUTS_MOUSE_MOTION, &msg, sizeof(msg)); return SPICE_SEND_PACKET(&spice.scInputs, msg);
} }
// ============================================================================ // ============================================================================
@ -1452,11 +1449,13 @@ bool spice_mouse_press(uint32_t button)
case SPICE_MOUSE_BUTTON_RIGHT : spice.mouse.buttonState |= SPICE_MOUSE_BUTTON_MASK_RIGHT ; break; case SPICE_MOUSE_BUTTON_RIGHT : spice.mouse.buttonState |= SPICE_MOUSE_BUTTON_MASK_RIGHT ; break;
} }
SpiceMsgcMousePress msg; SpiceMsgcMousePress * msg =
msg.button = button; SPICE_PACKET(SPICE_MSGC_INPUTS_MOUSE_PRESS, SpiceMsgcMousePress, 0);
msg.button_state = spice.mouse.buttonState;
return spice_write_msg(&spice.scInputs, SPICE_MSGC_INPUTS_MOUSE_PRESS, &msg, sizeof(msg)); msg->button = button;
msg->button_state = spice.mouse.buttonState;
return SPICE_SEND_PACKET(&spice.scInputs, msg);
} }
// ============================================================================ // ============================================================================
@ -1477,11 +1476,13 @@ bool spice_mouse_release(uint32_t button)
case SPICE_MOUSE_BUTTON_RIGHT : spice.mouse.buttonState &= ~SPICE_MOUSE_BUTTON_MASK_RIGHT ; break; case SPICE_MOUSE_BUTTON_RIGHT : spice.mouse.buttonState &= ~SPICE_MOUSE_BUTTON_MASK_RIGHT ; break;
} }
SpiceMsgcMouseRelease msg; SpiceMsgcMouseRelease * msg =
msg.button = button; SPICE_PACKET(SPICE_MSGC_INPUTS_MOUSE_RELEASE, SpiceMsgcMouseRelease, 0);
msg.button_state = spice.mouse.buttonState;
return spice_write_msg(&spice.scInputs, SPICE_MSGC_INPUTS_MOUSE_RELEASE, &msg, sizeof(msg)); msg->button = button;
msg->button_state = spice.mouse.buttonState;
return SPICE_SEND_PACKET(&spice.scInputs, msg);
} }
// ============================================================================ // ============================================================================