Basic command control structure.

[jkroonza]

This really is non-functional at this stage, but should illustrate the idea. No socket handling just yet. This is just the command parser idea and dispatch, looking for feedback.

[jkroonza]

@dfskoll this is a conceptual command parser. The idea is not to be highly efficient. Basically the root (which I should probably call cmd_root) lists commands, such as set and show, show then in this case already has a sub-command, and for illustration, some code to output the number of bound interfaces, and to list them.

For example to implement the draining options then one can add a sub-command under set such as

{ .command = "drain", .handler = handle_drain, }

Where the handle_drain function can then iterate the remaining options and set flags as appropriate, or one could even create that as a set of sub-commands, and the idea is to create generic setters for ints, char*s etc ... specifics unknown at this stage.

Next up I'm probably going to implement some kind of socket mechanism as per your suggestion, most likely just a unix socket, although from the little I've played with that I don't actually like a unix socket, but if I can use nc to interact with pppoe-server that's a win (it won't have line-control but we will have a working interface with fairly little work, and it should then be possible to at a later stage add line editing and even auto-complete if say one of the top-level commands is "complete" the CLI tool can then issue "complete show" which at this stage will simply output two lines "status" and "-" (or any terminator) such that readline can have lookup. That's very far down the line and I'm not convinced it's worth the effort currently.

Will require some argument to activate, eg -c /var/run/pppoe-server.sock or something.

[jkroonza]

@dfskoll completely independent from the upgrade process. This is my attempt at creating the discussed control socket. Still need to parse the received command and dispatch it to the various handler functions (thus still incomplete, and quit is hardcoded because, well nc), but I believe the concept should be clear at this stage.

[dfskoll]

On Tue, 27 Sep 2022 23:51:58 -0700 Jaco Kroon @.***> wrote:

Conceptually otherwise? I think the basic things I want to implement still as part of this PR are:

Conceptually, it's fine. I didn't take a very deep look at the code itself; I'll probably hold off until the PR is closer to being ready.

One thing you might want to consider... rather than rolling your own command interpreter, maybe embed an open-source one like Tcl or Lua? That can save you a fair bit of work while giving you a lot more flexibility. (Although it will save work in the long run, it probably is a bit more work up-front.)

The commands you describe all look reasonable.

Regards,

Dianne.

[jkroonza]

One thing you might want to consider... rather than rolling your own command interpreter, maybe embed an open-source one like Tcl or Lua? That can save you a fair bit of work while giving you a lot more flexibility. (Although it will save work in the long run, it probably is a bit more work up-front.)

I did not even consider this. This may well be true but in the short term to be honest it's as you say simpler to just roll my own. What I've got going is very simple, split the incoming line into words, supporting only "" at word beginning ending, so these are all valid single words:

• foobar
• foo"bar
• "foo bar"
• "foo\" bar"

These will fail to parse:

• "foo"bar" <-- inner " must be preceded by \ (will actually give an error).

Then it's simply processing the words beginning to end, traversing the tree as I go, or processing remaining words as arguments to the "final" command. So I'll likely need to write a function that can take a sequence of words and parse them into a struct of sorts such that parameter parsing is centralized and simplified for actions that require this. Would also make a "complete" command viable longer term eventually if we do write a cli client (the lack of readline() in nc is already getting to me and tab completion or ? would be most helpful, but I don't think that should stop us moving forward on this).

[jkroonza]

Hi @dfskoll

I would like to add a void argument to EventTcp_CreateAcceptor which will then be passed as a third argument to the EventTcpAcceptFunc callback. I'm aware however, that you've also got code that may be impacted by such a change - how big of a problem would that be for you, such a change would definitely require changes to all consumers of the function? Alternatively I need to add the void to the EventTcp

From my reading of event_tcp.c:

1. WriteBuf will free the returned EventTcpState* upon writing being complete (after calling the IO finished function). I do however not see that the EventHandler is removed from the EventSelector at all - I'm assuming I need to do this in the callback? Failing to remove this will plainly result in a segfault since next call to handle_writeable will have the state variable as created in EventTcp_WriteBuf passed as an invalid pointer if not removed.

2. Should I wish to add more data to the write-buf, how safe is it to adjust the EventTcpState internals? Ie, realloc(.buf, update .cur accordingly, add more bytes and update .len)? That's one way of appending lines for output, the other is to use a form of "double buffer".

I'm certain there were other questions, and I'm sure I can figure out a mechanism to deal with the above, but I'd prefer to rather check in with you first as I'm sure you have existing preferences/recommendations.

[jkroonza]

ping @dfskoll

[dfskoll]

Hi, Jaco,

Sorry... I have been very distracted and missed this message.

Re: The void * argument to EventTcp_CreateAcceptor, what extra info are you planning on passing around and how will it be used? You don't typically call EventTcp_CreateAcceptor very often and usually only during initialization. I don't have a problem in principle with adding the argument.

Also, at the end of writing, the event handler is removed because free_state is called which deletes the handler:

static void
free_state(EventTcpState *state)
{
    if (!state) return;
    EVENT_DEBUG(("tcp_free_state(state=%p)\n", state));
    if (state->buf) free(state->buf);
    if (state->eh) Event_DelHandler(state->es, state->eh); // <--- NOTE
    free(state);
}

Re: Adding more data to the write buffer: In principle, if you're very careful with book-keeping, it should be OK, but I would not recommend it because it relies on timing... what if writing finishes before you have a chance to add more to the buffer? Then the EventTcpState object will be destroyed.

If you could explain the scenario where you'd want to add to the write buffer, I might be able to suggest an alternative way to do it.

Regards,

Dianne.

[dfskoll]

Also, if you want to see a much better example of libevent in action, you might want to look at mailmunge-multiplexor.c in https://git.skoll.ca/Skollsoft-Public/mailmunge

[jkroonza]

Hi Dianne,

Sorry... I have been very distracted and missed this message.

No worries - thus the ping :).

Re: The void * argument to EventTcp_CreateAcceptor, what extra info are you planning on passing around and how will it be used? You don't typically call EventTcp_CreateAcceptor very often and usually only during initialization. I don't have a problem in principle with adding the argument.

I'm looking to avoid a global variable in control_socket.c such that other components (pppoe-relay daemon?) could re-use the code here and potentially have multiple control sockets (one is perhaps more privileged than another, so one could be "query only" to be able to provide active connection/status data to some info daemon/webpage - for example, a php-fpm instance that has permissions to open the info-socket to issue something like "status" or just outright get that info). As such control_socket_reader needs a void* to know what the command context is. These are merely ideas I have, not compulsory, so a global in control_socket.c that gets set on control_socket_init() is probably fine, it'll just fail on being called for a second time.

Also, at the end of writing, the event handler is removed because free_state is called which deletes the handler:

static void
free_state(EventTcpState *state)
{
    if (!state) return;
    EVENT_DEBUG(("tcp_free_state(state=%p)\n", state));
    if (state->buf) free(state->buf);
    if (state->eh) Event_DelHandler(state->es, state->eh); // <--- NOTE
    free(state);
}

Re: Adding more data to the write buffer: In principle, if you're very careful with book-keeping, it should be OK, but I would not recommend it because it relies on timing... what if writing finishes before you have a chance to add more to the buffer? Then the EventTcpState object will be destroyed.

if rp-pppoe was multi-threaded this would be a bigger issue, but since this is all single-threaded I don't see that there should be a major problem.

If you could explain the scenario where you'd want to add to the write buffer, I might be able to suggest an alternative way to do it.

If you look in the current implementation of control_socket.c and the functions invoked by it it issues a sequence of blocking write() calls via cs_printf(). This would now need to become a buffer, so one idea is to have cs_printf() just create a linked list of stuff to be written (double buffer), the other idea is to just inject it straight into the existing buffer that's being written (basically append and adjust length). This should be safe since we're single-threaded.

[jkroonza]

a6d8a55d (Dianne Skoll      2021-12-10 10:50:23 -0500 127) *  data -- extra data to pass to f.

Looks like you already started on the extra void* journey, or at least, that was the intention since that is part of the "initial commit".

[dfskoll]

Ah, I see the comment with data. I must have put it in initially and then removed it, having found no use for it. So sure, we can add it back in... but that will require some rework to the lower-level libevent stuff. Right now, I use the data field to hold the function needed by the Event_Tcp* functions. We'll either have to wrap the function and its data into a new structure that we pass as the data field to the lower level, or add a new extra_data fields to the lower-level structure.

[dfskoll]

On Tue, 06 Dec 2022 02:18:32 -0800 Jaco Kroon @.***> wrote:

[...]

I'm looking to avoid a global variable in control_socket.c such that other components (pppoe-relay daemon?) could re-use the code here and potentially have multiple control sockets (one is perhaps more privileged than another, so one could be "query only" to be able to provide active connection/status data to some info daemon/webpage - for example, a php-fpm instance that has permissions to open the info-socket to issue something like "status" or just outright get that info). As such control_socket_reader needs a void* to know what the command context is. These are merely ideas I have, not compulsory, so a global in control_socket.c that gets set on control_socket_init() is probably fine, it'll just fail on being called for a second time.

OK, I see. The way Mailmunge does this (exactly the same use case... a privileged and unprivleged socket) is to create two different control sockets that call different handler functions, like this:

    // Privileged socket
    if (!EventTcp_CreateAcceptor(es, sock, handleAccept)) { ... }

    // Unprivileged socket
    if (!EventTcp_CreateAcceptor(es, unpriv_sock, handleUnprivAccept)) { ... }

Then when we read form the socket using EventTcp_ReadBuf, handleAccept passes a NULL data argument and handleUnprivAccept a non-NULL argument so we know if it's a privileged or unprivileged socket.

Regards,

Dianne.

[jkroonza]

Ah, I see the comment with data. I must have put it in initially and then removed it, having found no use for it. So sure, we can add it back in... but that will require some rework to the lower-level libevent stuff. Right now, I use the data field to hold the function needed by the Event_Tcp* functions. We'll either have to wrap the function and its data into a new structure that we pass as the data field to the lower level, or add a new extra_data fields to the lower-level structure.

This was the easy part :). What is turning out to be harder, is after internal discussion for interface configuration we'd prefer something similar to the cisco style "interface context" (our naming) such that we enter commands as:

int eth0
mtu 1500
pppd options file /etc/ppp/options.whatever
activate
exit

rather than:

int eth0 mtu 1500
int eth0 pppd options file /etc/ppp/options.whatever
int eth0 activate

And one staff member is also looking to be able to do things like:

int *
int bond0.154 bond0.174 bond0.175.1 bond0.175.2

Which requires being able to change the base command structure with a custom void for sub-commands (in addition to the per-command void). This is shaping up nicely now.

I saw how mailmunge handled it, and in the context that seems perfectly fine, given my strategy you would have been able to use the same acceptor and just give a null vs non-null values to the accept void* data argument which it can just associate with the accepted connection. mailmunge code did help quite a bit on usage of the API though, thanks for that reference.

[dfskoll]

On Tue, 06 Dec 2022 07:04:56 -0800 Jaco Kroon @.***> wrote:

This was the easy part :). What is turning out to be harder, is after internal discussion for interface configuration we'd prefer something similar to the cisco style "interface context" (our naming) such that we enter commands as:

Ah... then you need to keep state somewhere until the "exit" command is read. Not impossible; just a bit of book-keeping.

Regards,

Dianne.

[jkroonza]

Dianne,

The context push/pop isn't tested yet, but I don't see any reason why that shouldn't work. Still need to verify for memory leaks etc ... but I believe this may be more to your liking. Will get this through valgrind tomorrow hopefully, for now I need sleep.

"show status" should work at this stage, whilst set will drive people nuts since there are no sub-options available (I should probably comment/remove this prior to merge ... but basically I used that to test the completion options when just typing s).

Bring up the socket with -U /tmp/foo.sock to pppoe-server.

Connect using nc -U /tmp/foo.sock

It's not friendly, but it's workable.

[jkroonza]

valgrind summary (after execute show status followed by quit):

==1733== LEAK SUMMARY:
==1733==    definitely lost: 0 bytes in 0 blocks
==1733==    indirectly lost: 0 bytes in 0 blocks
==1733==      possibly lost: 0 bytes in 0 blocks
==1733==    still reachable: 5,958 bytes in 9 blocks
==1733==         suppressed: 0 bytes in 0 blocks

Most of those seems to relate to SET_STRING stuff not getting free'd, so one-off leaks, there are a handful others but again only one-off stuff from what I can see, the only new introductions before/after my code are these:

==1733== 14 bytes in 1 blocks are still reachable in loss record 1 of 9
==1733==    at 0x48417E5: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==1733==    by 0x491401A: strdup (strdup.c:42)
==1733==    by 0x10DFDE: main (pppoe-server.c:1519)

==1733== 16 bytes in 1 blocks are still reachable in loss record 2 of 9
==1733==    at 0x48417E5: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==1733==    by 0x11616E: EventTcp_CreateAcceptor (event_tcp.c:152)
==1733==    by 0x114D79: control_socket_init (control_socket.c:336)
==1733==    by 0x10EAF6: main (pppoe-server.c:1705)

==1733== 48 bytes in 1 blocks are still reachable in loss record 4 of 9
==1733==    at 0x48417E5: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==1733==    by 0x115174: Event_AddHandler (event.c:255)
==1733==    by 0x114D79: control_socket_init (control_socket.c:336)
==1733==    by 0x10EAF6: main (pppoe-server.c:1705)

The first one is SET_STRING() for the unix_socket path name.

The second is for the acceptor which remains in the event selector until exit (ie, socket is never explicitly shut down).

The last is actually from CreateAcceptor for same, but this frame is eliminated by way of gcc stack optimization (replacing stack frame for CreaetAcceptor with that of AddHandler).

These can be fixed if you prefer, and I can probably fix most of the others too, but frankly, I don't think they're a problem since as you say, we keep them until shutdown.

[jkroonza]

Added code for "set drain on|off|quit".

[jkroonza]

ping @dfskoll

I personally think this is ready, but I'm waiting for you to confirm if you're happy before building further things on top of this.

[dfskoll]

Hi, Jaco,

The code looks fine to me (modulo a couple of minor comments.) One thing we can't forget is to update the man pages, though. :) The control socket and all the supported commands should be documented, either in the pppoe-server man page or possibly in a separate pppoe-control man page if you think that would be more appropriate.

Regards,

Dianne.

[dfskoll]

And... nice work!

[jkroonza]

appreciate the feedback, sorting out a related radius issue tonight then I'll get back to this in the week still.

[jkroonza]

Switched to using vasprintf in printErr (on the assumption this is better than what I originally had it as, refer to the discussion above).

Documentation updated (new commit).

[dfskoll]

OK, vasprintf is fine too. The error-printing function isn't (or shouldn't be) speed-critical, so I'd have been OK with two calls to the %-formatting functions, but I have no strong feelings either way.

[dfskoll]

This is looking good. I can merge it whenever you are ready... either now, or later if you want to add more commands first. Thank you!

[jkroonza]

This is looking good. I can merge it whenever you are ready... either now, or later if you want to add more commands first. Thank you!

I was wondering about whether to add more onto this or just keep it at this to keep the review size more manageable. Logically I think this PR is about as big as it should get. My opinion.

For future patches I think "one command per commit", or "logical group into a commit" kind of thing, and depending on complexity of individual diffs a set of patches per PR or just a single logical change. Your thoughts?

Currently on my working set there is two patches I believe to be ready to go, but they still need final testing (encountered whilst performing radius related work where I require pppoe-server to pass remotenumber to pppd):

1. consolidate user- and kernel-space implementations into a single start, with the varying paramters being calculated into the vargs array.
2. Pass remotenumber mac to pppd (which will then include that as Calling-Station-Id into the radius request).

I've yet to create a successful pppoe connection with that, which is why I've not pushed that yet. Most of those delays are due to radius side not wanting to do what I want it to do just yet but I think I got down to the root problem now and should finalize that tomorrow.

[dfskoll]

On Tue, 13 Dec 2022 07:08:32 -0800 Jaco Kroon @.***> wrote:

I was wondering about whether to add more onto this or just keep it at this to keep the review size more manageable. Logically I think this PR is about as big as it should get. My opinion.

OK. Just send me an "ack" and I will merge.

For future patches I think "one command per commit", or "logical group into a commit" kind of thing, and depending on complexity of individual diffs a set of patches per PR or just a single logical change. Your thoughts?

I think new commands can all go into one PR. Other changes that aren't related to new commands might be better in a separate PR.

1. consolidate user- and kernel-space implementations into a single start, with the varying paramters being calculated into the vargs array. 2. Pass remotenumber mac to pppd (which will then include that as Calling-Station-Id into the radius request).

OK.

I've yet to create a successful pppoe connection with that, which is why I've not pushed that yet. Most of those delays are due to radius side not wanting to do what I want it to do just yet but I think I got down to the root problem now and should finalize that tomorrow.

Cool!

Regards,

Dianne.

[jkroonza]

@dfskoll ACK

[dfskoll]

Merged; thanks.