Make executor state available to the harness V2 #1900
Conversation
|
With this implementation, GenericInProcessExecutor stores (in addition to its internal state) the state of the parent structure (like |
which part does this? i only see QemuExecutorWithState calling executor_state |
| // Crash and timeout hah | ||
| hooks: (InProcessHooks, HT), | ||
| phantom: PhantomData<(S, *const H)>, | ||
| inner: GenericInProcessExecutorInner<HT, OT, S>, |
There was a problem hiding this comment.
what was the reason that you have to make this inner?
i think i heard once but forgot
There was a problem hiding this comment.
It's to avoid code duplication between GenericInProcessExecutor and GenericInProcessExecutorWithState.
GenericInProcessExecutorWithState does, sorry I didn't make it clear. |
| /// The inmem executor simply calls a target function, then returns afterwards. | ||
| /// The harness can access the internal state of the executor. | ||
| #[allow(dead_code)] | ||
| pub struct GenericInProcessExecutorWithState<H, HB, HT, OT, S, ES> |
There was a problem hiding this comment.
I think you should name this so that it ends in State.
Because you already have executor's state. so it's confusing to determine if this state or executor
There was a problem hiding this comment.
Yes I defnitely need to rename those guys. For now it's just for the PoC
There was a problem hiding this comment.
Maybe something like MutableInProcessExecutor?
There was a problem hiding this comment.
GenericInProcessWithStateExecutor?
There was a problem hiding this comment.
Definitely better than my original proposition. Not sure if embedding WithState in the struct name is a good naming tho, since it collides with the name of the different states.
| harness_fn: HB, | ||
| /// The state used as argument of the harness | ||
| pub executor_state: ES, | ||
| /// Inner state of the executor |
There was a problem hiding this comment.
so it's not state it's more
Inner executor with state right?
There was a problem hiding this comment.
I was thinking of putting it in inner, to merge the different states in the same struct.
It would also let me remove this ugly pub.
Ok. Will it feel less odd if you rename |
Not sure, since it suggests it represents the state of QEMU itself, which is not the case |
|
But your QemuExecutorState has just How about naming it to |
|
I applied the renaming propositions. |
Alternative to #1847, without changing
Executor.