[Home]

Summary:ASTERISK-00119: [patch] Manager Protocol Ambiguous For Long Responses [Post 1.0]
Reporter:jayson (jayson)Labels:
Date Opened:2003-08-18 01:21:33Date Closed:2011-06-07 14:10:10
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) mgr123.patch
( 1) mgr123-2.patch
Description:Certain Actions return arbitrarily long responses that do not fit the format for the protocol (requiring extensive special casing to implement).  This should be handled explicitly by the protocol.

****** ADDITIONAL INFORMATION ******

My beef lies with the Command action.  It's response comes as:

Response: Follows
Many
Lines
Of
Ill
Formatted
Text
--END COMMAND--

This currently is only an issue with the Command action, but we will likely encounter it later.

Specifically, we have a very elegant protocol for communicating events/requests (it's even symmetrical!), but we kludge in long responses.

I propose one of two ways of making a standard response format.  Either break it into lines and return multiple Events (like Status).  This would also benefit from a concrete closure event (ala bug ASTERISK-118).

It may also be useful to define a standard way to encode the data as either plain or Base64 (to allow binary data).  

While individual events and Base64 for this could waste space, I see no other good way to:

a) prevent jamming the manager pipeline with a large, atomic response
b) potentially return binary data
c) allow for long-running responses (perhaps some sort of AGI over manager-protocol become possible)
d) allow other responses to be interspersed with output (ties in with a/b/c and scalability)

Right now we seem to handle this case-by-case.  I would recommend defining it in the protocol before someone develops important software that depends on this (thus making a compatability/stable-interface concern).
Comments:By: jayson (jayson) 2003-08-18 01:22:42

"I propose one of two ways of making a standard response format.  Either break it into lines and return multiple Events"

should read

"I propose a standard response format.  Break it into lines and return multiple Events"

edited on: 08-18-03 01:07

By: jayson (jayson) 2003-08-18 01:28:42

Eventually, I'd like to see something like the following (possibly interspersed with events and other responses):

Action: Command
Command: show manager commands
ActionID: 12345

Response: Partial
Encoding: Base64
Base64: <base64 line 1>
ActionID: 12345

Response: Partial
Encoding: Base64
Base64: <base64 line 2>
ActionID: 12345

Response: Partial
Encoding: Base64
Base64: <base64 line 3>
ActionID: 12345

Response: Complete
ActionID: 12345

edited on: 08-18-03 01:12

By: jayson (jayson) 2003-08-18 01:31:32

Also, consider RequestedEncoding and EncodingTypes of plain and Base64 to begin with.

I think we could really benefit from a strong management protocol.  This kind of thing would give us some initial robustness that will save headaches later.

By: Mark Spencer (markster) 2003-08-18 16:14:30

Yucky.  The reason it's broken into lines is not because it's long, but because that's the way the output is.  Essentially we're following a system similar to how MIME divides sections.  What kind of response would "jam the manager pipeline"?  And why on earth would we want to intersperse other events with waiting for a command response to come back (which should be quick)?

Any time you want to add any complexity to Asterisk, you have to convince me that the benefit of that complexity far outweighs the (rather high) value that I personally place on simple, direct, and well understood interfaces.  Encoding command responses in base64 definitely seems like an unnecessary thing to do.

By: jayson (jayson) 2003-08-18 21:54:31

Base64 is perhaps a bit heavy.

As for command responses, what about using the System command.  Is there a guarantee of quickness there?  Why limit ourselves to quick management traffic?

I actually don't want to enable Asterisk's implementation it intersperse those events, I just want to engineer the protocol so that we have the option to if it is important later.  This is nothing about changing the backend now and everything about not breaking the protocol later.

I'm looking for a way to keep parsing simple.  Right now, any command that returns '\r\n\r\n' before the --END COMMAND-- risks breaking a simple parser.  Now it requires special casing to know which commands give a long response and what to watch for at the end.  A standard for this would solve that.

By: Mark Spencer (markster) 2003-08-19 01:02:39

Adding a separate event (and associated overhead) for each line seems ill advised.  Again, I'm not interested in adding functionality just so that "it can be there in case we think we might need it in the future."  Show that it's a good idea now, or don't worry about it.  

You're obviously a very creative and clever person.  However, I have clearly failed to convince you of the danger of feature creep.  Perhaps a review of the history of SIP will make you see what happens when you try to make a single protocol which is everything to everyone, not just in terms of feature but architecture, and when you carried away with trying to design overly flexible and vague protocols.  Next thing you know, someone's going to want the management interface exposed as XML or have some other tragically elegant, expensive, and catastrophic extension.

By: jayson (jayson) 2003-08-19 17:13:51

But we could make everything XML!!!  (That's a joke.)

Actually, I'm in touch with what you mean.  Right now, we have a command that needs to dump in arbitrarily long strings of stuff.  Right now, it is done as a special case.  I would like to either:

a) make the existing behavior a standard (i.e. hold new commands that do similar things do it the same way)
-or-
b) come up with a way to do it in a general/easily parsable way
-or-
c) adjust it to fit mechanisms we already have (events, which also give the bonus of make it asynchronous)

I do fear feature creep.  What bothers me isn't the "feature" of having an overly complex (overfeatured) spec that is impossible to implement (SIP, X.500, Ada).  What bothers me is having three different actions do the same thing three different ways.   THAT is feature creep.

This isn't the same as bug ASTERISK-117.  We actually have ONE action (Command) that does this.  I just would like for it to be standardized.

If we do settle on (a), I'd like to meet the following criteria:

1)  Make the ending marked very standard (either the same for all actions, or generated from the action name in a case sensitive way)
2)  Prevent two adjacent newlines before the end of the the response (so the parser still has a clear idea where records end)
3)  Possibly prepend a header-like prefix onto each line (prevents 2 being a problem, wasteful, also creates empty headers which are explicitly invalid ala bug ASTERISK-117)

I'm flexible on number 3 (it's just an idea).

I'm not trying to add features to the protocol, I'm just trying to rid it of abiguities (like disappearing empty headers and data that just gets spit out in the middle of an otherwise very parsable protocol stream).

By: Mark Spencer (markster) 2003-08-20 02:01:43

Well, how about the following:

1) If Response header is "Follows" then the message continues until ending token.  

2) Response must be the last header in such messages.

3) Ending token is "--END COMMAND-<actionid>-" where <actionid> represents the action id specified in the request (if specified).  If no action id is specified, the termination shall be "--END COMMAND--".

4) Ending token will always be followed by two \r\n pairs.

How does that sound?

By: jayson (jayson) 2003-08-20 11:05:37

Actually, the ActionID idea is probably overkill (although very MIMEy).

I'd be fine with --END COMMAND-- as long as when we come out with Action: Foo the end token is --END FOO--.

I hate to break current compatability, but I just thought of this:

Action: Command
Command: PrintSomeStuff

Response: Success
Message: Follows
:Some
:Stuff
:
:
:Notice the two newlines that are neatly escaped.
:This is the last line.



The above has the advantage of being relatively low overhead, easy to implement, and dead simple to parse.

I just want to avoid have situations like this:

Action: Command
Command: PrintSomeStuff

Response: Success
Message: Follows
Two Embedded Newlines


--END COMMAND--



That forces me to have my parser do special handling if I'm inside a command response.  Right now, all I have to do is read input until a \r\n\r\n pair, then chew up the block you've read.  There's a clear point when it is known to be fruitful to start parsing.  Without this layout, I've got to be constantly examining the input stream.  HTTP solves this with extended headers, but I don't really like that (whitespace games).

By: jayson (jayson) 2003-08-20 11:10:22

There is also a big benefit if the parsing ever gets out of sync.  If we don't escape this data somehow, we may end up with our manager trying to parse a long output.

As a practicality concern, I propose that the above only be used IFF:

a) Message: Follows appears
b) Immediately after the above
c) No headers follow it in that request

In other words, these are all *INVALID*:

Response: Success
Message: Follows
ActionID: 1234
:a
:b
:c


Response: Success
Message: Is In Here Somewhere
:a
:b
:c


Response: Success
Message: Follows
:a
:b
:c
ActionID: Stuff

I'm flexible on (b) and (c), but it seems like a good KISS (Keep it simple s*) situation.

By: jayson (jayson) 2003-08-20 11:12:45

Note, I'm also updating the manager documentation as I do these updates.

I'm trying to avoid a monolithic patch, but this stuff is all kind of intertwined (I've got Bugs ASTERISK-117 and ASTERISK-118 done; I'm testing it a bit before submission).

By: Mark Spencer (markster) 2003-08-20 19:50:23

I don't like the idea of breaking compatibility, but in this case, i think your ":" suggestion is a good one.  I think putting the ActionID in the middle of the -END COMMAND-- would allow a paranoid application to ensure that there was no attack involved with a command placing "--END COMMAND--" in the response.  However, with your ":" prefix, you also prevent this possibility.  Feel free to submit this patch and I'll take it.  You'll also need to update gastman and astman's parsers to support it (as well as support the old format, at least for the time being) before I can apply it.

Finally be sure to include yourself in the CREDITS (unless you don't want to be included for some reason) with you patch submission since your patches represent some substantial gains to Asterisk's management interface.

And just give up on bug 121!

By: jayson (jayson) 2003-08-21 09:01:22

Thank you.

As for Bug ASTERISK-117, could you close it?  I don't have the power (from this end) to make it go away (I'll note on that bug to do so as well).

I'll get to patching.

I'll look at astman (I've already patched it for the StatusComplete event), but I don't think it uses Command, does it?  I'll check up on that.  Gastman does, so I'll do it.

How should I tell the new response from the old one?

a) a leading colon on the next line (could be problematic in fringe cases)?
b) add Format: LongResponse header?
c) Change protocol version number (1.1)?

By: Mark Spencer (markster) 2003-08-21 10:30:03

Part of bug 121 still remains though -- the ActionID.  I did agree with adding it so if you want to submit that as part of your patch I'd be happy to.

astman does not use command, but its parser should probably understand whats going on anyway for completeness (and because people may use it as a reference).

You can tell the old one from the new one by the absense or presence of a ":" as the first character.  If there is a ":" then it's the new one otherwise it's the old one.  Eventually we can ditch support for the old one, so probably best to make it a Makefile configurable option.

By: jayson (jayson) 2003-08-21 21:02:50

<SHORT-VERSION>
In short (not my strong point), everything uses ast_cli to abstract printing.  This is good.  However, ast_cli only allows us to pass around a file descriptor.  That is bad.  It means there is virtually no way to mangle output for things like prefixing a colon to the output of a command.  My proposed changes would be massively invasive to implement.  A core API would have to change.  I also see no good way to transition from one API to another--it would have to be a watershed event.  You would probably not accept such a massive patch.
</SHORT-VERSION>

<LONG-VERSION>
Ironically, I waited until last to implement the prefixing part (the rest of the patching is done!!!).

Well, this looks nearly impossible to implement without a major API change of one sort or another.  As I'm sure you probably remember, EVERYTHING uses ast_cli to printf-ify its output.

On the surface, it looks very simple:

int ast_cli_command(int fd, char *s);

Give it a file descriptor and it worries about printing (in a buffer-overflow safe way, no less).  However, the key to its flexiblity (file descriptor allows printing to console or manager) is also the problem.

I can think of no solid way of filtering that file descriptor.  Specifically, prefixing a colon to the output of the command, requires intervention at the ast_cli level at a thousand points in the code.  In fact, not only do we need to intervene there, it may not even be possible.  There are nightmare scenarios like this:

1. Manager receives Command: Foo
2. action_command calls ast_cli_command w/ management fd
3. ast_cli_command looks up handler from cli (uses find_cli to find it)
4. then call various, sundry handlers
5. Handler calls ast_cli to print
6. Sometimes this even prints multiple lines per invocation

It looks like it could, at first, be possible to simply (with a lot of work) adjust ast_cli to accept something like this:

struct ast_cli_stream {
 int fd;
 char prefix[32];
}

void ast_cli(int fd, char *fmt, ...);

This is not a small change to fix up.  ast_cli calls appear to be present in at least these files:

./apps/app_agi.c
./apps/app_meetme.c
./apps/app_queue.c
./asterisk.c
./astmm.c
./channels/chan_agent.c
./channels/chan_alsa.c
./channels/chan_h323.c
./channels/chan_iax.c
./channels/chan_iax2.c
./channels/chan_local.c
./channels/chan_mgcp.c
./channels/chan_oss.c
./channels/chan_sip.c
./channels/chan_zap.c
./cli.c
./db.c
./frame.c
./image.c
./manager.c
./pbx.c
./pbx/pbx_config.c
./res/res_crypto.c
./res/res_indications.c
./res/res_parking.c
./translate.c

To the tune of 329 references.  It appears to be used for the manager interface and the command line interface, but I can't rule out it is doing some other magic formatting somewhere.  Besides adjusting function calls and such, I would have to find the source of every fd that comes in to be printed so that I could wrap it in one of our structures.

The task is mammoth.

Billions of patches later, we would seemingly be safe and done (and invasive as hell for what we're trying to do).  However, we still lose due to subtle situations like:

ast_cli(stream, "Embedded\r\nNewlines");

That means I effectively need to hack the string to pieces to print it out.  Something like:

char output[4096];
char *tag = output;
char *oldtag = output;
va_list ap;

va_start(ap, fmt);
vsnprintf(output, sizeof(stuff), fmt, ap);
va_end(ap);

while ( tag = strstr(output,"\r\n") ) {
 *tag = '\0';
 tag += 2;
 write(stream->fd,stream->prefix,strlen(stream->prefix));
 write(stream->fd,oldtag,strlen(oldtag));
 write(stream->fd,"\r\n",2);
 oldtag = tag;
}

Note that this abstraction puts all of the communications data into a single struct and all of the output handling in one function (you previously had the latter but not the former).  This paves the way for later actually implementing handlers for different output types like you've done with the cli commands and manager commands.  This probably doesn't sway you much.  :)

In other words, since almost every core asterisk component, every channel, and every application directly use this in some way or the other, there is no practicable way to output other than directly to a file descriptor (with no intervening modification).

I don't currently believe that, if I were to adjust the thousand lines of code required to do this, that you would accept the patch.  From our previous conversations, I think you would probably indicate that it was too much change (and too invasive of an API change) for such little gain.

I would probably argue that it would be useful to have the capability to abstract our communications beyond the file descriptor level.  I would say that Asterisk will probably need this sooner or later.  I would cry that we will have problems with this forever because we cannot do the most rudimentary of filtering, isolation, escaping, and processing of our output.  Likely, I would fall short of having any convincing practical applications other than Action: Command.  I suspect that would be the end of it.

I would really like to fix this.  I acknowledge that it is not really broken.  I understand that what we gain from this (concretely, now) is very small.  I believe strongly that this will get in the way later.  I would really like to fix it now.  I also know that I have shown little to you that should make you trust me to do this.  I know that doing this will probably be very rocky for other developers trying to work on the everyday code.  As such, if I were you, I probably would not let me do it.

I really hope that I'm wrong.  Am I?  
</LONG-VERSION>

edited on: 08-21-03 20:56

edited on: 08-21-03 20:58

By: Mark Spencer (markster) 2003-08-21 23:56:15

Okay, so we abandon the ":" prefix idea and go back to the original idea.

By: jayson (jayson) 2003-08-22 09:00:36

I thought you would say that.

If I made the patch anyways, would you accept it?

It's mostly an API change (just a *REALLY* commonly used API).

By: Mark Spencer (markster) 2003-08-22 09:08:13

If you made which patch?  The big one described in here?  If so, probably not before 1.0...  I'll have to think about it since it's a lot more overhead and i'm not sure it can't be implemented in less intrusive ways.

By: jayson (jayson) 2003-08-22 09:38:06

After trying to convince myself that I couldn't do it and that it wasn't worth it, I made a test program with the above code.  It works pretty well.  The only difficulty is that ast_cli (which really is used far beyond the CLI) is used everywhere.

I think I could do it in a solid night of coding.

The only extra (runtime) overhead is chopping the lines up at the newlines.  I can prevent this from being a problem by special-casing the general case where there is no prefix.

I'll implement it and, if necessary, maintain it outside the CVS tree.  I just would like to know it could be integrated in, say, a six month timeline.  More than that is probably more than I can commit to.

I think the complexity is worth it.  If I'm wrong, I'll find out when I give up.

Is that acceptable?

By: Mark Spencer (markster) 2003-08-22 09:42:29

If it doesn't go into "1.0" then I'll be leary of changing the protocol.  It would at that point have to be requested, e.g.:

Action: Connect Options
Prefix: yes

Or some such.  Other than that, no problem.

By: jayson (jayson) 2003-08-22 10:08:09

After the API change, such a thing will be trivial.

I'd actually rather have it go in as a protocol revision.  Say Asterisk Manager/1.1 (or something).

I would rather not have ostensibly the same version of the protocol behave differently.  Also, when we eventually reach protocol version 3 or 4, it would be cleaner to deprecate a whole old protocol (with all of the backwards compatability problems we've accrued).  Think HTTP 1.0 versus HTTP 1.1.  No major changes, but some very different behavior based on the version (pipelining, et cetera.).

We'll burn that bridge when I get there...

Thank you.

By: Mark Spencer (markster) 2003-08-24 18:31:58

What's the status of your patch?

By: jayson (jayson) 2003-08-27 12:14:49

About 45%.

I'm currently taking it slow (just replacing the existing fd references with a structure that wraps the fd).

I've gotten manager.c, pbx.c, cli.c, asterisk.c, and a few others.

I'll probably have the whole thing patched soon.  Then, I'll have to do a cvs update and see what's changed (currently about 3 days behind).

By: jayson (jayson) 2003-09-07 16:50:12

Had to restart this.  I hit a small snag relating to the way you pass around a FD at one point (casting to void and all).  Rethought things a bit and came up with a cleaner implementation.  Was at about 60%.

Since I'm needing to restart here.  I'm going to implement the fixes to those other two bugs FIRST so that we can get the bug count down and so that that behavior can make it into 1.0.

Will attempt to keep you posted.

Note that the new implementation no longer requires a complete rewrite of everything at once.  Instead it allows incremental updates.  All of the functionality I'm attempting to get won't work until it is all finished--but Asterisk won't be completely broken in the meantime.

If you want more explanation.  Ask.

Thanks,

Jayson

By: John Todd (jtodd) 2003-10-16 04:23:27

<poke, poke>  Jayson: any free time recently to put more time into this, by any chance?

By: Brian West (bkw918) 2004-01-06 23:55:17.000-0600

Any updates on this?

By: jrollyson (jrollyson) 2004-01-28 00:28:46.000-0600

Still waiting for update on status.

By: jayson (jayson) 2004-01-28 09:21:20.000-0600

I've been putting time into it, but the problem is so interwoven into stuff, that I can't keep it current with CVS.  I've had to scrap the codebase twice already because it stomped on someone else's changes.  Unfortunately, there's not really a good way to do this too incrementally.

I'm getting a block of time coming up this week, so I'll see if I can get a partial patch posted to this ticket.

By: jayson (jayson) 2004-01-28 21:31:55.000-0600

Okay, I had an absolutely massive epiphany while chugging through this API change.

Since the whole goal of this is to finalize the protocol before 1.0 (not the internal interface), I think I've found a way to cut back from fixing the interface to pass around some sort of stream_handle instead of an fd (my previous approach).

For the record, I still think we should do the above, as passing around fds the way we do is SCARY!

Anyways, I got to thinking that I could set up an array indexed on file descriptors and use it to store information about whether or not to prefix a certain fd.  In other words, instead of passing around a stream that knows to prefix itself, instead have the cli function do the prefixing after looking up the fd.

Obviously this isn't perfect.  It still doesn't correctly handle cases where something in the cli (via the mgr interface) writes DIRECTLY to the fd.  However, there are only a few cases of this that I think we can let go in the name of sanity.  Look at how the handlers in res/res_crypto.c pass an fd into try_load_keys (or something) and then pass the fds directly to the key handling functions.  It's completely breaks down here (although this implementation is strange enough that we may consider another way of doing things).  Also, this problem highlights the potential need to handle input streams and well as output streams (as it cavalierly passes the fd hitherto used for output and uses it for input apparently).

After all of this, I embarked on making a patch.  I quickly ran into a problem.  In order to do the substitution, I needed to not only find newlines in a formatted string (and prefix the line after them) but I needed to keep track of newlines that terminate (or don't terminate) a previous string.  I can't just prefix before every print or prefix after each newline as that won't correctly handle instances where I don't end up printing any more before dropping prefix mode (ends up with a spurious prefix).  I also had to special case the first iteration of the prefix handling in case a completely null string was passed in (otherwise a spurious prefix was generated).  To do this, I allocated another array to keep track of whether we had ended on a newline (and thus, should prefix *IF* we are printing something).

Currently, the patch works quite well for the initially targeted function (Action: Command).  Otherwise, it doesn't actually touch much and it's delightfully self contained.

Here are the current FIXMEs:

FIXME:  I allocated arrays at 16384 and put a LOG_WARNING on the bounds checking for setting them.  I am not sure how high fds can get for a single process in Linux so I currently consuming 32kB in buffers (which may either be grossly too much or too little).

FIXME:  This fails to handle situations where the fd is written to.  We still need to fix the use of the ast_cli interface, but we probably should do so post 1.0.

FIXME:  This needs more testing.  I've tested it by hand ten or twenty times, but I still think it needs to be tested.  Do enough people use the mgr interface with Action: Command that it would be noticed/testable?  I can test it in my system but the load on this will still be light.

In the interests of getting this done (and also the interest of seeing everyone barf over my horendous hack of code), I'm attaching the patch that implements this.

I don't know why, but I have a feeling that this is pretty solid.  I played with pointer math but I can't figure out what could go wrong.  I hope everyone else experiences that too.  :)

Enjoy.

By: dmartz (dmartz) 2004-01-30 22:25:46.000-0600

Jayson, this is great stuff.

I just read over this thread, and I'm wondering if anyone is addressing the tabular responses yet? Example: zap show channels

I'm sure someone has addressed this somewhere, but I haven't found anything yet. I've came up with a few items:

- One column could spill into another
- Column width changes between revisions would certainly mess with a parser
- The occational tabular listing ends with "X widgets found", and some either provide a listing or "0 widgets found", but most listing only gaurentee a header.

I'm working on a C# call manager application right now, so my concern is that I'll need to alter the parsing logic at any future build.

Thoughts?

By: jayson (jayson) 2004-02-02 10:42:12.000-0600

The way we handle this internally is kind of a mess.  Internally, there is a function called ast_cli that does printf style, buffer-overflow safe, formatting and then writes the result to a file descriptor.  Originally, it was intended for formatting and then printing to the console, but it's general utility caused it to be adopted as a general formatting interface across the entire codebase.

Now that we need to modify some data in transit (or reformat depending on where it is to go), this is less than ideal.  To give you an idea of the state of the system, raw file descriptors are tossed around all over the place, get written to by ast_cli, or worse, get written to directly!

The interface between the cli functions (zap show channels example) and the manager is pretty crude (passes around a file descriptor).  In fact, the cli commands have no way to know whether they're talking to a management interface or a console.  While this is a good thing, the downside is that they still do pretty much direct IO.

To coherently implement something like I suspect you want, we'll need a system that understands how to pass around lots of dynamic, heterogenous data structures representing tabular data, commands, headers, and the like.  In C# or Python or Java, that would be pretty easy.  In C with this codebase, I think that sounds like a Asterisk 2.0 feature.

Not that it's a bad idea, I think it's the direction we should go.  I just think that the nature of the code makes this improbable until after 1.0 (when Mark might give me or somebody free reign to break things with a few patches to finally get this fixed).

Jayson

By: dmartz (dmartz) 2004-02-02 13:36:29.000-0600

Perhaps we could at least be consistant in the tabular format then. Some report a summary quantity of zero but skip the summary if there is data, others always report the summary quantity despite the data, and I think there is one that skips the header if there is no data. That will help a bit in parsing.

Hey Jayson, what about putting a tab character (\t) seperating the columns as an extension to your current work. For the console the character is removed, for management the character is passes (or optionally passed).

The internals would send something like "Zap/1\tMy Name   \t". You are already processing the string in the cli functions. I suppose its an all or nothing approach requiring the "all" to be in implemented in every piece of CLI code.

Well, my main point is having consistant headers and footers on tabular output.

Darren

By: jayson (jayson) 2004-02-02 17:45:17.000-0600

Yeah.  I hear that.

This extension really only has the purpose of attaching an arbitrary text payload to each protocol request.  Like a CDATA in and XML stream.  I only really did it to clean up the protocol for the Command action (although other things could use it too).  The above could be done independant of the protocol, so I think it deserves another bug.  I'll probably tackle it eventually.  During cleaning up ast_cli would be a good time.  Since that's my goal after 1.0 comes out, I'll be sure to keep this in mind.

By: jayson (jayson) 2004-02-02 18:23:17.000-0600

Noticed a few slight inconsistencies in my patch.  I've made some changes that actually make it more useful in the process.

Specifically, the first patch would prepend a colon to payload lines.  While this made it parseable, my purpose for choosing a colon was to allow payload lines to function like lines with an "empty key".  That way, you could write your implementation to split on a colon, and lines without keys are your payload.

The problem was, the manager protocol actually specifies headers be delimited with a colon FOLLOWED BY a space.  I adjusted the prefix system to allow prefixing with arbitrary strings instead of single characters.  This may have increased the memory footprint somewhat, however, I can now print each line appropriately prefixed so as to keep the protocol clean.

I also removed the --END COMMAND-- string.  The prefixes conveyed this information nicely.  Since we were already breaking compatibility with the old Command syntax (not apparently widely used), I figured it would be good to remove the other cruft now.

I also updated the manager protocol document to reflect this change.  My wording may not be best, so I welcome any comments.

The new patch will be attached as mgr123-2.patch

By: jayson (jayson) 2004-02-02 18:42:37.000-0600

ast_man doesn't use Command anywhere.  I don't really see that it's parser cares about this.  Exactly what would you want me to update to support this?

The input_check function actually will parse this already.  The existing get_header function which should neatly ignore this change.  I can implement a get_payload (or get_body) function to grab a response like this.  Nothing would use it initially.  A get_body_first_line and get_body_next_line function pair would be childs play.  If you're happy with the API, I could do the latter in about 30 minutes.  Let me know.

Jayson

By: jayson (jayson) 2004-03-01 16:31:24.000-0600

I've been using this in solid production for a month now.

It's proven to be rock solid for me.

Any problems with applying the patch?

By: Olle Johansson (oej) 2004-03-25 17:17:01.000-0600

The general standing seems to be that this big a change has to wait to be considered after 1.0 release. We're closing it pending 1.0 release with [Post1.0] marker to catch it afterwards. Please bug a bug marshal when you want it re-considered.

By: Olle Johansson (oej) 2004-03-25 17:18:13.000-0600

---To be re-evaluated after 1.0 release, closing temporarily --