On 19/06/2021 20:53, Vladimir Sementsov-Ogievskiy wrote:
14.06.2021 10:33, Emanuele Giuseppe Esposito wrote:
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -52,29 +52,35 @@ typedef struct BlockCopyCallState {
/* Coroutine where async block-copy is running */
Coroutine *co;
- /* To reference all call states from BlockCopyState */
- QLIST_ENTRY(BlockCopyCallState) list;
-
/* State */
- int ret;
bool finished;
- QemuCoSleep sleep;
- bool cancelled;
+ QemuCoSleep sleep; /* TODO: protect API with a lock */
+
+ /* To reference all call states from BlockCopyState */
+ QLIST_ENTRY(BlockCopyCallState) list;
/* OUT parameters */
+ bool cancelled;
bool error_is_read;
+ int ret;
Hmm, about that. Is @ret an "OUT parameter"? Yes it is.
But someone may think, that out parameters doesn't need locking like
"State" parameters (otherwise, what is the difference for the person who
read these comments?). But that is wrong. And ret is modified under
mutex for reason.
In patch 5 I added a comment above @ret and @error_is_read:
/* Fields protected by lock in BlockCopyState */
I can add your explanation too.
Actually, the full description of "ret" field usage may look as follows:
Set concurrently by tasks under mutex. Only set once by first failed
task (and untouched if no task failed).
After finish (if call_state->finished is true) not modified anymore and
may be read safely without mutex.
So, before finished, ret is a kind of "State" too: it is both read and
written by tasks.
This shows to me that dividing fields into "IN", "State" and "OUT",
doesn't really help here. In this series we use different policies of
concurrent access to fields: some are accessed only under mutex, other
has more complex usage scenario (like this @ret), some needs atomic access.
Yes but I think especially the IN vs State division helps a lot to
understand what needs a lock and what doesn't.
Emanuele