[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH V2 17/23] xen/ioreq: Introduce domain_has_ioreq_server()




On 11.11.20 15:27, Jan Beulich wrote:

Hi Jan.


    }

    #define GET_IOREQ_SERVER(d, id) \
diff --git a/xen/include/xen/ioreq.h b/xen/include/xen/ioreq.h
index 7b03ab5..0679fef 100644
--- a/xen/include/xen/ioreq.h
+++ b/xen/include/xen/ioreq.h
@@ -55,6 +55,20 @@ struct ioreq_server {
        uint8_t                bufioreq_handling;
    };

+#ifdef CONFIG_IOREQ_SERVER
+static inline bool domain_has_ioreq_server(const struct domain *d)
+{
+    ASSERT((current->domain == d) || atomic_read(&d->pause_count));
+
This seems like an odd place to put such an assertion.
I might miss something or interpreted incorrectly but these asserts are
the result of how I understood the review comment on previous version [1].

I will copy a comment here for the convenience:
"This is safe only when d == current->domain and it's not paused,
or when they're distinct and d is paused. Otherwise the result is
stale before the caller can inspect it. This wants documenting by
at least a comment, but perhaps better by suitable ASSERT()s."
The way his reply was worded, I think Paul was wondering about the
place where you put the assertion, not what you actually assert.
Shall I put the assertion at the call sites of this helper instead?
Since Paul raised the question, I expect this is a question to him
rather than me?
Yes, it was primarily a question to Paul, but I wanted to hear your opinion as well. Sorry for the confusion.

--
Regards,

Oleksandr Tyshchenko




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.