On 11.11.20 18:28, Paul Durrant wrote:
Hi Paul.
d->ioreq_server.server[id] = s;
+
+ if ( s )
+ d->ioreq_server.nr_servers++;
+ else
+ d->ioreq_server.nr_servers--;
}
#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?
If it is indeed a question for me then yes, put the assertion where it is clear
why it is needed. domain_has_ioreq_server() is essentially a trivial accessor
function; it's not the appropriate place.
Got it. Thank you for the clarification.
--
Regards,
Oleksandr Tyshchenko