[libvirt] [PATCH 6/9] rpc: client: incapsulate error checks
Michal Privoznik
mprivozn at redhat.com
Fri Feb 8 16:19:49 UTC 2019
On 2/7/19 1:58 PM, Nikolay Shirokovskiy wrote:
> 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 at virtuozzo.com>
> ---
> src/libvirt_remote.syms | 3 ++-
> src/remote/remote_driver.c | 16 ----------------
> src/rpc/virnetclient.c | 4 ++--
> src/rpc/virnetclientstream.c | 45 ++++++++++++++++++++++++++++++++++----------
> src/rpc/virnetclientstream.h | 5 ++++-
> 5 files changed, 43 insertions(+), 30 deletions(-)
>
> diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
> index 88745f2..98586d1 100644
> --- a/src/libvirt_remote.syms
> +++ b/src/libvirt_remote.syms
> @@ -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;
> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index 058e4c9..1ff55e2 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -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;
> diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
> index fcc2e80..70192a9 100644
> --- a/src/rpc/virnetclient.c
> +++ b/src/rpc/virnetclient.c
> @@ -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;
> diff --git a/src/rpc/virnetclientstream.c b/src/rpc/virnetclientstream.c
> index 136ed16..a7a7824 100644
> --- a/src/rpc/virnetclientstream.c
> +++ b/src/rpc/virnetclientstream.c
> @@ -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 */
This sounds very fishy. There should be one exact lock that needs to be
locked. I understand why you've written it this way though. It's because
virNetClientStreamRecvPacket() unlocks the stream just before calling
virNetClientSendStream() which then locks the client.
I'll post a separate patch for that.
> +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,7 +492,9 @@ int virNetClientStreamRecvPacket(virNetClientStreamPtr st,
> virObjectLock(st);
>
> reread:
> - if (!st->rx && !st->incomingEOF) {
> + if (virNetClientStreamCheckState(st) < 0) {
> + goto cleanup;
> + } else if (!st->rx && !st->incomingEOF) {
I'd prefer if these were two separate if-s. Also, ultimately this would
be a while() or do-while() loop.
> virNetMessagePtr msg;
> int ret;
>
Michal
More information about the libvir-list
mailing list