External/libvirt
1
0
Fork 0
mirror of https://gitlab.com/libvirt/libvirt.git synced 2024-10-05 22:05:47 +00:00
Code Issues Packages Projects Releases Wiki Activity

rpc: client: incapsulate error checks

Browse Source 
Checking virNetClientStreamRaiseError without client lock
is racy which is fixed in [1] for example. Thus let's remove such checks
when we are sending message to server. And in other cases
(like virNetClientStreamRecvHole for example) let's move the check
into client stream code.

virNetClientStreamRecvPacket already have stream lock so we could
introduce another error checking function like virNetClientStreamRaiseErrorLocked
but as error is set when both client and stream lock are hold we
can remove locking from virNetClientStreamRaiseError because all
callers hold either client or stream lock.

Also let's split virNetClientStreamRaiseErrorLocked into checking
state function and checking message send status function. They are
same yet.

[1] 1b6a29c21: rpc: fix race on stream abort/finish and server side abort

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
This commit is contained in:
Nikolay Shirokovskiy 2019-02-07 15:58:44 +03:00 committed by Michal Privoznik
parent 8457fd5034
commit ad063f6192
5 changed files with 43 additions and 29 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
src/libvirt_remote.syms
View File

@ -54,6 +54,8 @@ virNetClientProgramNew;
# rpc/virnetclientstream.h
virNetClientStreamCheckSendStatus;
virNetClientStreamCheckState;
virNetClientStreamEOF;
virNetClientStreamEventAddCallback;
virNetClientStreamEventRemoveCallback;
@ -61,7 +63,6 @@ virNetClientStreamEventUpdateCallback;
virNetClientStreamMatches;
virNetClientStreamNew;
virNetClientStreamQueuePacket;
virNetClientStreamRaiseError;
virNetClientStreamRecvHole;
virNetClientStreamRecvPacket;
virNetClientStreamSendHole;

16
src/remote/remote_driver.c
View File

@ -5600,9 +5600,6 @@ remoteStreamSend(virStreamPtr st,
virNetClientStreamPtr privst = st->privateData;
int rv;
if (virNetClientStreamRaiseError(privst))
return -1;
remoteDriverLock(priv);
priv->localUses++;
remoteDriverUnlock(priv);
@ -5634,9 +5631,6 @@ remoteStreamRecvFlags(virStreamPtr st,
virCheckFlags(VIR_STREAM_RECV_STOP_AT_HOLE, -1);
if (virNetClientStreamRaiseError(privst))
return -1;
remoteDriverLock(priv);
priv->localUses++;
remoteDriverUnlock(priv);
@ -5676,9 +5670,6 @@ remoteStreamSendHole(virStreamPtr st,
virNetClientStreamPtr privst = st->privateData;
int rv;
if (virNetClientStreamRaiseError(privst))
return -1;
remoteDriverLock(priv);
priv->localUses++;
remoteDriverUnlock(priv);
@ -5709,9 +5700,6 @@ remoteStreamRecvHole(virStreamPtr st,
virCheckFlags(0, -1);
if (virNetClientStreamRaiseError(privst))
return -1;
remoteDriverLock(priv);
priv->localUses++;
remoteDriverUnlock(priv);
@ -5834,9 +5822,6 @@ remoteStreamCloseInt(virStreamPtr st, bool streamAbort)
remoteDriverLock(priv);
if (virNetClientStreamRaiseError(privst))
goto cleanup;
priv->localUses++;
remoteDriverUnlock(priv);
@ -5849,7 +5834,6 @@ remoteStreamCloseInt(virStreamPtr st, bool streamAbort)
remoteDriverLock(priv);
priv->localUses--;
cleanup:
virNetClientRemoveStream(priv->client, privst);
virObjectUnref(privst);
st->privateData = NULL;

4
src/rpc/virnetclient.c
View File

@ -2193,7 +2193,7 @@ int virNetClientSendStream(virNetClientPtr client,
virObjectLock(client);
if (virNetClientStreamRaiseError(st))
if (virNetClientStreamCheckState(st) < 0)
goto cleanup;
/* Check for EOF only if we are going to wait for incoming data */
@ -2205,7 +2205,7 @@ int virNetClientSendStream(virNetClientPtr client,
if (virNetClientSendInternal(client, msg, expectReply, false) < 0)
goto cleanup;
if (virNetClientStreamRaiseError(st))
if (virNetClientStreamCheckSendStatus(st, msg) < 0)
goto cleanup;
ret = 0;

44
src/rpc/virnetclientstream.c
View File

@ -184,14 +184,9 @@ bool virNetClientStreamMatches(virNetClientStreamPtr st,
}
bool virNetClientStreamRaiseError(virNetClientStreamPtr st)
static
void virNetClientStreamRaiseError(virNetClientStreamPtr st)
{
virObjectLock(st);
if (st->err.code == VIR_ERR_OK) {
virObjectUnlock(st);
return false;
}
virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__,
st->err.domain,
st->err.code,
@ -202,8 +197,31 @@ bool virNetClientStreamRaiseError(virNetClientStreamPtr st)
st->err.int1,
st->err.int2,
"%s", st->err.message ? st->err.message : _("Unknown error"));
virObjectUnlock(st);
return true;
}
/* MUST be called under stream or client lock */
int virNetClientStreamCheckState(virNetClientStreamPtr st)
{
if (st->err.code != VIR_ERR_OK) {
virNetClientStreamRaiseError(st);
return -1;
}
return 0;
}
/* MUST be called under stream or client lock */
int virNetClientStreamCheckSendStatus(virNetClientStreamPtr st,
virNetMessagePtr msg ATTRIBUTE_UNUSED)
{
if (st->err.code != VIR_ERR_OK) {
virNetClientStreamRaiseError(st);
return -1;
}
return 0;
}
@ -474,6 +492,9 @@ int virNetClientStreamRecvPacket(virNetClientStreamPtr st,
virObjectLock(st);
reread:
if (virNetClientStreamCheckState(st) < 0)
goto cleanup;
if (!st->rx && !st->incomingEOF) {
virNetMessagePtr msg;
int ret;
@ -646,6 +667,11 @@ virNetClientStreamRecvHole(virNetClientPtr client ATTRIBUTE_UNUSED,
virObjectLock(st);
if (virNetClientStreamCheckState(st) < 0) {
virObjectUnlock(st);
return -1;
}
*length = st->holeLength;
st->holeLength = 0;

5
src/rpc/virnetclientstream.h
View File

@ -36,7 +36,10 @@ virNetClientStreamPtr virNetClientStreamNew(virStreamPtr stream,
unsigned serial,
bool allowSkip);
bool virNetClientStreamRaiseError(virNetClientStreamPtr st);
int virNetClientStreamCheckState(virNetClientStreamPtr st);
int virNetClientStreamCheckSendStatus(virNetClientStreamPtr st,
virNetMessagePtr msg);
int virNetClientStreamSetError(virNetClientStreamPtr st,
virNetMessagePtr msg);