Re: [libvirt] [PATCH 6/9] rpc: client: incapsulate error checks

Fri, 08 Feb 2019 08:20:18 -0800 

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 <nshirokovs...@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Reply via email to