[Tomasz Moń <desowin@xxxxxxxxx>]

On Tue, Aug 23, 2022 at 6:24 PM Jirka Novak <j.novak@xxxxxxxxxxxx> wrote:
> > The problem with GenerateConsoleCtrlEvent() is that the caller has to
> > be attached to the target process console. While we could technically
> > do so, it requires freeing any already open console because process
> > can be attached to at most one console. The pretty much only sane
> > solution to the problem is to have a helper program between Wireshark
> > and extcap.
> >
> > The helper would simply spawn extcap with provided parameters and
> > accept commands from Wireshark e.g. on pipe. The commands would be to
> > gracefully terminate (CTRL_C_EVENT or CTRL_BREAK_EVENT) and forcefully
> > terminate (TerminateProcess()). Note that the helper must not be
> > forcefully terminated as it would leave the extcap running.
> >
> > While far from ideal, I think the helper is the only sensible
> > approach. Note that GLib gspawn-win32-helper does something different,
> > so going back to the GLib helper is not what this is all about.
>
> Even I understand reasons, it looks to me very complex. I'm sorry, but
> my Win32 skills are too low to make it.

Nobody says that you have to write it yourself. I understand your
desire to have things available quickly, but I believe this should be
part of the gradual extcap refinement that is ongoing (and led to
seemingly unrelated changes like the leaking handles fixes, GLib event
loop integration and significant mmdbresolve speedup) and not just
slapped together.

> The proposal described above combines two approaches - windows signals
> and windows pipe.
> What I see as missing in discussion is that even extcap will receive
> "just" windows signal, extcap must be adapted to handle graceful
> shutdown. And sequence will be:
>
> wireshark - pipe - helper - signal - extcap
>
> and to achieve it, we have to modify wireshark, write helper and modify
> extcap.

The pipe in this case is not part of the interface and can be freely
altered, even removed if necessary, *without* requiring any change on
extcap side.

> Extcap have to be adapted only if it requires graceful shutdown. But if
> it requires it, it must be adapted. If it will not be adapted (do not
> require graceful shutdown), it will be terminated as nowadays.

There is a difference between ExitProcess() (this is what default
CTRL_C_EVENT and CTRL_BREAK_EVENT does) and TerminateProcess() (this
is what Wireshark has been doing and still does today). The helper
would have to fallback to TerminateProcess() only if the extcap does
free the console (which is deliberate action).

Another major difference is that CTRL_C_EVENT and CTRL_BREAK_EVENT get
sent to all processes sharing the console. This is particularly useful
if extcap creates new processes. Just calling TerminateProcess() on
extcap handle is not necessarily giving the (grand-)children any
chance to notice things are shutting down.

> Won't be easier to say, that when author have to modify extcap, they
> will use the pipe?

The assumption here is that using the pipe is simple. I don't think
such assumption is correct.

> Yes, it is a little more work on extcap side, but it must be touched
> anyway. With pipe we will save a lot of effort on wireshark/helper and
> complexity of overall solution will be much lower.

The point is things can improve without modifying extcap if the extcap
did not resort to nasty hacks due to legacy reasons (e.g. claiming to
be Windows subsystem application to prevent flashing console window
during extcap initialization).

> Proposal with pipe I made is backward compatible so when extcap is not
> modified to support graceful shutdown, it do not expose support of pipe.
> Therefore wereshark do not create pipe, do not pass it to extcap and
> terminate it as before.

The problem is that if the pipe is part of the interface it will have
to stay there even if it becomes a major pain to maintain.

> One more note...
> *nix world use signals and graceful shutdown is already implemented on
> wireshark side. When extcap needs graceful shutdown, it should be
> modified to process SIGTERM.
> If the extcap should work on Windows too, extcap must support Windows
> signals CTRL_BREAK_EVENT in addition to SIGTERM. The code is similar,
> but there are differences so code must support two ways how to stop it.

Not every extcap has to be cross-platform. There are extcaps that are
Windows only and there is absolutely no sense to port them to UNIX
systems.

> If we will use pipe, it can be used on *nix and Windows world same way
> (except #ifdef to use correct API calls). Isn't it better?

This question incorrectly assumes that every extcap is written in C
and writing cross-platform code that uses pipes is easy.

There is absolutely no reason to use shutdown pipe on UNIX systems
where there is perfectly fine mechanism (SIGTERM) that has been used
for decades.

Being able to test extcap outside Wireshark (e.g. just run extcap,
passing normal filename instead of extcap pcap pipe, in Windows cmd)
and verify that CTRL+C works correctly sounds appealing to me. With
the pipe it won't be so easy.

Best Regards,
Tomasz Moń