[PATCH for-4.15] tools/xenstored: liveupdate: Properly check long transaction

[Julien Grall <julien@xen.org>]

From: Julien Grall <jgrall@amazon.com>

As XenStored is single-threaded, conn->ta_start_time will always be
smaller than now. As we substract the latter from the former, it means
a transaction will never be considered long running.

Invert the two operands of the substraction in both lu_reject_reason()
and lu_check_allowed(). In addition to that, the former also needs to
check that conn->ta_start_time is not 0 (i.e the transaction is not
active).

Take the opportunity to document the return condition of
lu_check_allowed().

Fixes: e04e53a5be20 ("tools/xenstore: allow live update only with no transaction active")
Reported-by: Bjoern Doebel <doebel@amazon.de>
Signed-off-by: Julien Grall <jgrall@amazon.com>

---

I am a bit puzzled on how -F is implemented. From my understanding we
will force LiveUpdate when one of the following conditions is met:
  1) All the active transactions are long running
  2) If we didn't manage to LiveUpdate after N sec

It is not quite clear why we need the both as 2) would indirectly cover
1). However 2) is probably unsafe as we may reset transactions for
"well-behaving" guest.

So I am thinking to send a patch to drop 2). Any opinions?

This patch is candidate for 4.15. Without it, the long-running
transactions are not properly accounted.
---
 tools/xenstore/xenstored_control.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/tools/xenstore/xenstored_control.c b/tools/xenstore/xenstored_control.c
index d7ad112138b2..8e470f2b2056 100644
--- a/tools/xenstore/xenstored_control.c
+++ b/tools/xenstore/xenstored_control.c
@@ -459,11 +459,18 @@ static bool lu_check_lu_allowed(void)
 	list_for_each_entry(conn, &connections, list) {
 		if (conn->ta_start_time) {
 			ta_total++;
-			if (conn->ta_start_time - now >= lu_status->timeout)
+			if (now - conn->ta_start_time >= lu_status->timeout)
 				ta_long++;
 		}
 	}
 
+	/*
+	 * Allow LiveUpdate if one of the following conditions is met:
+	 *	- There is no active transactions
+	 *	- All transactions are long running (e.g. they have been
+	 *	active for more than lu_status->timeout sec) and the admin as
+	 *	requested to force the operation.
+	 */
 	return ta_total ? (lu_status->force && ta_long == ta_total) : true;
 }
 
@@ -474,11 +481,12 @@ static const char *lu_reject_reason(const void *ctx)
 	time_t now = time(NULL);
 
 	list_for_each_entry(conn, &connections, list) {
-		if (conn->ta_start_time - now >= lu_status->timeout) {
+		if (conn->ta_start_time &&
+		    (now - conn->ta_start_time >= lu_status->timeout)) {
 			ret = talloc_asprintf(ctx, "%s\nDomain %u: %ld s",
 					      ret ? : "Domains with long running transactions:",
 					      conn->id,
-					      conn->ta_start_time - now);
+					      now - conn->ta_start_time);
 		}
 	}
 
-- 
2.17.1