[squid-dev] [PATCH] helper process library

[Alex Rousskov]

On 06/18/2017 12:08 PM, Amos Jeffries wrote:
> Implement a library for C++ helpers to link against which provides
> a main() function to achieve generic startup/query/shutdown debug
> information and and a simpler API for processing a query line.

Executive summary: I believe the proposed library interface is harmful
on several levels and must be redesigned. The fix is easy though.

Specific problems are discussed below, followed by the fix suggestion.

I doubt we should add a library that defines main(). There is nearly no
real advantage in doing so, and it introduces a significant surprise
factor for developers that expect every program to define its main(). It
also adds a serious hurdle for any library-using code that needs to
customize its "main" operation. If you want to encapsulate the main()
logic, you may provide Main() or equivalent, but there is a better way
to accomplish what you want as detailed further below.

I do not think we should add a library that declares functions that it
does not define. For example, HelperProcess::Start() and
HelperProcess::Options() are declared in libhelper_process but are then
defined in various helpers. This introduces a big surprise for
developers (and development tools!). AFAICT, this is nothing but a hack
to make it possible to define main() in the library, which is the wrong
goal on its own, as discussed above.

It feels like you are trying hard to avoid defining a proper
Helper::Instance or Helper::Application class that declares the common
interface and implements the bits that it can. That would be the right
strategy to avoid code duplication without any hacks, surprises,
compatibility issues, and extensibility problems.

BTW, with the class-based approach, the main() function in each helper
can be reduced to a single line:

int
main(int argc, char *argv[])
{
    return MyInstance().run(argc, argv);
}


> + * Copyright (c) 2017, Treehouse Networks Ltd. New Zealand

Please move that copyright statement into CREDITS, like we try to do
with nearly all other custom copyright statements. The usual reasoning
applies, plus there is hardly anything to copyright in those files!


> === added file 'src/helper/Process.h'

> +namespace HelperProcess {

I suggest s/Process/Program/ or s/Process/Instance/ because the general
code you are adding exists for the helper as a whole while a helper
instance using your code may create other helper processes (that will
not use the Start/Options/DoOneRequest API). Your call.


> +    while (getline(std::cin,buf)) { // will return false at EOF
> +        ndebug(HelperProcess::Name << "| Got " << buf.length() << " bytes '" << buf << "' from Squid");
> +        HelperProcess::DoOneRequest(buf);
> +    }

This implementation assumes that each helper requests contains a single
line. That assumption is not general. Some helpers (e.g., the
certificate generator helper) use multiline requests. You can avoid or
even solve this problem if the above loop becomes a Helper::App method
calling a virtual getRequestBuffer() function (with a default getline()
implementation).


If you make another patch revision, please post an --ignore-all-spaces
diff or equivalent to highlight the important changes.


Thank you,

Alex.