On 09/14, Roland McGrath wrote: > > > Basically, we have ev_push() and ev_pop(), that is all. > > I don't mean it's hard to understand what the code does. > It's clear enough that it's a simple ring buffer scheme. > > But just having that extra data structure is extra complexity and storage > even so. It's quite overly general for what is really needed.
First of all, I am not sure we need this abstraction layer too. It was quickly added after a sleepless night when I realized my previous plan was hopelessly wrong. See also the end this email... I'd like to keep it for now, because it is easier to me to change the code this way. For example, ptrace_report_syscall_exit() does not have to know anything about the previous state, it just pushes the new state. And if we should do something on resume I can place the related code close to callback's code, this way it is immediately obvious (to me!) what's going on. The problem is, when I started this work, I knew nothing about what ptrace actually does. I only understood ptrace_stop/signals part. Now I am trying to "clone" the current behaviour looking at the current code, and I never know what I can miss and what should be changed later. > > We need to record the values for task->exit_code and task->ptrace_message > > at least. _Perhaps_, we also need to record the bit from context->options, > > I am not sure. > > Do you really need to store more than one ptrace_message word? I don't > think so. Let's think about the cases. Where is there a "stacked" event > that has a message word? I think that's only PTRACE_EVENT_VFORK_DONE. Again, I am not sure we need more than one ptrace_message word. I just don't know, and I didn't even try to think about this. All I know now, ->ev_message is the simple way to correctly set ->ptrace_message. > Why do you think you might need to record any option bits? > The past state of option bits should not matter. (from [PATCH 37] "introduce ptrace_event->ev_options") > > When the tracee resumes from TASK_TRACED, context->options can be changed. > > > > Introduce ptrace_event->ev_options. If a callback sets ->ev_options != 0, > > do_ptrace_resume() checks that context->options matches or discards the > > event. > > IMHO this hair is a pretty clear indication that the "queued reports" model > is just not the right model. You have all this new complexity because of > the model that you queue the details of the report at event time, but in > reality much of that determination belongs at report time rather than event > time. Agreed. (and I thought about this too even before I added ->ev_array). But again, this way ptrace_resume() becomes very simple, it knows nothing about the event. All logic lives near the callback's code. And. Currently I am not 100% sure we can always figure out the next state during resume (if it wasn't recorded previously like this model does). > But I think you need to answer my earlier question about the motivation for > this whole plan. Once the code will work (or "mostly" work), and once I will be able to actually understand what I am trying to implement, I am going to remove this layer. Because yes, I think most probably you are right, the reality is simpler. (however: _perhaps_ jctl stops can add some complications, not sure). Perhaps I am wrong, but I think it is much simpler to simplify the working code, compared to improving/complicating the code which doesn't work. So, in short: ->ev_array was added just to make some progress. Not sure this was the wise choice though. Oleg.
