#emc-devel | Logs for 2007-01-14

[01:05:33] <jepler> alex_joni: no, I think there was more than one commit. let me look.
[01:06:19] <jepler> alex_joni: 1.102 is a bugfix for 'save netl'
[01:06:26] <jepler> alex_joni: and there's a doc change for it too
[01:07:07] <jepler> halcmd.c rev 1.19
[01:08:06] <jepler> and for the testsuite to pass, there's a change to tests/save.0/expected but I'm not sure if just merging rev 1.5 will make the test pass or not
[01:08:40] <jepler> yes, it probably will -- merge revs 1.4 and 1.5
[01:08:51] <jepler> (rev 1.3 was already merged to the branch)
[04:10:43] <jmkasunich> hmm.... replace_vars() in halvmd is vulnerable to buffer overflows I think
[04:17:26] <SWPadnos> could be
[04:17:34] <SWPadnos> it's a pretty stupid function
[04:18:24] <SWPadnos> in fact, there was a potential error, but I think jeff did something instead of making me fix it :)
[04:19:17] <jmkasunich> its certainly not easy to read
[04:19:32] <jmkasunich> but I don't think any other implementation would be easy to read either
[04:20:07] <jmkasunich> what concerns me is that if any substitutions are longer than the string they replace, the string grows
[04:20:17] <jmkasunich> and there is no check to keep it from outgrowing its buffer
[04:20:29] <SWPadnos> hmmm. I thought I had thought of that :)
[04:20:53] <jmkasunich> I'm willing to be convinced that you did...
[04:21:09] <SWPadnos> I'd have to look at it to convince myself first
[04:21:13] <jmkasunich> but there are several strcpy's there that seem unprotected to me
[04:22:17] <jmkasunich> actually, they are strncpys, but the n is not the destination buffer size
[04:22:33] <SWPadnos> no, it should decrease as the pointer is advanced
[04:22:47] <SWPadnos> destination buffer size only works the first time
[04:23:09] <jmkasunich> yes, but I mean n isn't related to the output buffer size at all
[04:23:32] <jmkasunich> for instance, the very first strncpy
[04:23:36] <SWPadnos> it should have started at the buffer size, and go down as the buffer gets filled (but that's just me thinking of how it *should* be done)
[04:24:04] <jmkasunich> copies the string from the start of the source to the first delimiter, regardless of the size of the output buffer
[04:24:17] <jmkasunich> if there is no delimiter, it copies the whole thing
[04:24:52] <jmkasunich> the function itself doesn't verify that it will fit
[04:25:18] <jmkasunich> for this particular copy it will, but only because the caller ensures that the dst buffer is bigger than the source
[04:25:59] <SWPLinux> strcspn will return strlen(input_str) if the delimiters aren't found
[04:26:13] <jmkasunich> right
[04:26:27] <jmkasunich> so if the input doesn't contain a delimiter, the whole thing will be copied to output
[04:26:38] <jmkasunich> that means the output buffer needs to be as big as the input (or bigger)
[04:26:38] <SWPLinux> I guess the assumption is that the source string isn't longer than MAX_CMD, and that the destination is at least MAX_CMD long
[04:26:45] <jmkasunich> but the function itself doesn't test for that
[04:26:50] <SWPLinux> true enough
[04:27:20] <jmkasunich> anyway, thats not the one that really worries me, because we know about the caller, and we know it doesn't pass a long src and a short dst
[04:27:30] <jmkasunich> strcat(dp, replacement);
[04:27:36] <jmkasunich> that worries me tho...
[04:28:02] <jmkasunich> $ set foo=sdfjalkhgdjasgkjhdfgjhdfspkhsdfkjghsdfkhsdpfkjhsdpfkhj
[04:28:06] <SWPLinux> yes
[04:28:07] <jmkasunich> halcmd $foo
[04:28:08] <SWPLinux> I see the issue
[04:29:15] <jmkasunich> to be honest, I'm not enamored with the approach (too many strlens, etc), and I'd probably implement it by doing a char-by-char walk thru the source string, but thats almost irrelevant
[04:29:45] <SWPLinux> that's what the str* functions do, only they're faster
[04:30:09] <jmkasunich> for a single function, they are
[04:30:31] <jmkasunich> when you use dozens of calls to differnt ones, and do lots of interesting things with the results, who knows
[04:30:39] <jmkasunich> anyway, speed is not the issue
[04:31:25] <SWPLinux> they all return a count. I don't think there's any reason to write the loops here
[04:31:46] <jmkasunich> thats not my point
[04:31:52] <SWPLinux> except that you can see exactly what the code is doing, without caring about the library
[04:31:53] <SWPLinux> :)
[04:32:01] <jmkasunich> I'd certainly use some of them
[04:33:38] <jmkasunich> example:
[04:33:39] <jmkasunich> next_delim=strcspn(varP, ")");
[04:33:38] <jmkasunich> if (next_delim > strlen(varP))/* error - no
[04:34:00] <jmkasunich> that loops thru the string twice, once doing the search, and once for the strlen
[04:34:26] <jmkasunich> how about next_delim=strcspn(varP, ")");
[04:34:57] <jmkasunich> if ( varP[next_delim] == '\0' ) /* error, no closing paren found
[04:35:30] <SWPLinux> it won't be 0, it'll be strlen(varP)
[04:36:00] <SWPLinux> err - nevermind :)
[04:37:44] <SWPLinux> that should work
[04:38:02] <SWPLinux> I'm trying to remember what the other problem was
[04:38:42] <jmkasunich> well, the piece I just talked about has nothing to do with the buffer overflows... that was just an illustration of my discomfort with the heavy use of strlen
[04:38:49] <SWPLinux> I understand
[04:39:49] <jmkasunich> would you be offended if I rewrote that function? or would you prefer to try to add overflow protection to the existing implementation?
[04:40:59] <SWPLinux> ah - I fixed that one. now I don't have to remember what it was
[04:41:06] <SWPLinux> I can add the overflow protection
[04:41:11] <SWPLinux> It's not offensive though
[04:41:34] <SWPLinux> I have an aversion to writing everything in assembly language disguised as C
[04:42:00] <jmkasunich> I understand that
[04:42:11] <jmkasunich> and I didn't want to do that
[04:42:29] <SWPLinux> anyway - I can fix it, unless you're just sitting around with nothing to do :)
[04:42:34] <jmkasunich> my comment about walking the pointers thru probably led the discussion the wrong direction
[04:42:44] <SWPLinux> no problem - we digress often
[04:43:57] <jmkasunich> I find the logic in the existing code hard to follow, and its non-trivial to calculate the ramaining space in the buffer... and since some places uses the n in strncpy already, you need more logic to make sure n is the lesser of strnlen (replacement) and spacein(buf)
[04:44:43] <SWPLinux> yeah - that's probably why I didn't do it right in the first place
[04:44:59] <jmkasunich> byte by byte eliminates some of that
[04:45:13] <SWPLinux> that's true
[04:45:16] <jmkasunich> or strncpy could be used, with n _always_ the space remaining in the buffer
[04:45:37] <jmkasunich> sorry, strncat, not cpy
[04:45:49] <SWPLinux> strn{whatever}
[04:46:20] <SWPLinux> I think I was using the 'n' versions because I didn't want to change the source string (though I may do it in some places anyeay)
[04:46:21] <SWPLinux> anyway
[04:47:38] <jmkasunich> another one: strncpy(var, varP, next_delim);
[04:48:04] <jmkasunich> next_delim is the length of the string you want to substitute, not the length of buffer var (which is 128)
[04:48:28] <jmkasunich> a perverse person could write halcmd show pin $dhjalsdhgksldhglksdjglkhfg<bunches more>sadfgldskg
[04:48:32] <jmkasunich> and overflow var
[04:48:33] <SWPLinux> yep
[04:49:22] <jmkasunich> its real tempting to malloc a copy of the original string, then you can feel free to modify it
[04:49:42] <SWPLinux> that would work. or just not mind modifying it
[04:49:51] <jmkasunich> for instance, use strspn or strcspn to find the end of a var name, and replace it with /0
[04:49:55] <SWPLinux> I thikn I left it alone so it could be printed in an error mesasge if desired
[04:50:21] <jmkasunich> then strncat(var, string, len_of_var) would work nicely
[04:50:32] <jmkasunich> oops, cpy, not cat
[04:50:41] <SWPLinux> heh
[04:50:44] <jmkasunich> seems I can't keep those 2 straight
[04:51:56] <jmkasunich> anyway, this all started as part of backporting the net command, but I wanted to review the code first
[04:52:06] <SWPLinux> makes sense
[04:52:08] <jmkasunich> its been a while since i hacked in halcmd, and I wanted to come back up to speed
[04:52:15] <jmkasunich> so I'm reviewing everything
[04:52:22] <SWPLinux> but hey - these count as bugfixes, so we can add them later ;)
[04:52:39] <jmkasunich> well, I intend to fix it in head, and then backport
[04:53:19] <jmkasunich> if you want to fix that function, I'll continue the review with the rest of the code
[04:53:43] <SWPLinux> sounds good to me
[04:54:34] <jmkasunich> I guess I'll have to figure that function's logic out after all (when I saw the overflows, I said to myself, "well, this is fskced, if you're gonna rewrite it anyway, don't worry about it"
[04:55:28] <SWPLinux> heh - that's one approach
[04:55:29] <jmkasunich> hmm, loopcount is incremented, but never read
[04:55:39] <SWPLinux> were you thinking of adding or subtracting anything from it?
[04:55:55] <SWPLinux> so it is
[04:56:02] <jmkasunich> functionality? no - the comment block describes exactly what it is supposed to do
[04:56:12] <jmkasunich> I was just gonna take a differnet approach to doing it
[04:56:36] <SWPLinux> I think I envisioned telling which token was the problem, hence the loopcount (that's my story and I'm sticking to it)
[04:56:53] <jmkasunich> actually, might add one more return value: -6 substituted string too large for buffer
[04:57:07] <SWPadnos> good idea
[04:57:15] <SWPadnos> "buffer overflow" - let's keep it cryptic
[04:57:29] <jmkasunich> ok
[04:57:55] <SWPadnos> heh - really cinfuse people: "out of memory" :)
[05:10:52] <jmkasunich> found more "dealing with perverse input" bugs, this time in some of my code
[05:11:12] <SWPLinux> bummer
[05:11:36] <jmkasunich> parse_cmd checks for comments, by testing if the first char (only) of a token is #
[05:11:59] <SWPLinux> so " #" doesn't count
[05:12:25] <jmkasunich> the space would be discarded during tokenisation
[05:12:35] <SWPLinux> oh, good
[05:12:54] <jmkasunich> but show pin# something would result in tokens "show" "pin#" and "something" and generate an error
[05:13:13] <jmkasunich> show pin#something would only make two tokens, but still an error
[05:13:22] <SWPadnos> hmmm
[05:13:44] <jmkasunich> the reason that doesn't actually happen is because replace_vars also looks for #
[05:14:08] <jmkasunich> however, it doesn't properly deal with # inside quoted strings - it truncates the line at a #, even if its quited
[05:14:13] <jmkasunich> quoted
[05:14:18] <SWPadnos> so the string should betruncated at teh #
[05:14:20] <SWPadnos> yep
[05:14:37] <SWPadnos> quotes are a pain, if they're relevant
[05:14:47] <jmkasunich> tokenise() properly deals with quoted strings
[05:14:56] <SWPadnos> yes
[05:15:31] <SWPadnos> you know - one thing that might be good would be to make a HAL_lib tokenize function
[05:15:56] <jmkasunich> what for?
[05:16:13] <SWPadnos> it's done in any driver that needs string "cfg" vars and halcmd
[05:16:23] <SWPadnos> could be useful elsewhere as well (I think)
[05:16:41] <SWPadnos> I'm not sure if it can be reasonably factored out though
[05:17:41] <jmkasunich> I actually wrote a fairly generic tokenizer for vcp, see src/hal/user_comps/vcp/tokenizer.h
[05:18:13] <SWPadnos> yeah. it seems like it should be a fairly generic thing
[05:18:18] <jmkasunich> not suitable for kernel space tho, uses malloc, and expects to be reading from a file, not a string
[05:18:21] <SWPadnos> you give it lists of tokens
[05:18:49] <SWPadnos> well, you can have a shell around the tokenizer itself (wexcept that vcp needs multiline tokens)
[05:21:03] <jmkasunich> anyway, back to halcmd (factoring out a tokenizer isn't a 2.1 thing)
[05:21:08] <SWPadnos> nope
[05:21:27] <jmkasunich> right now, the sequence is replace_vars, tokenize, parse
[05:21:44] <jmkasunich> comments were originally handled (incorrectly) in parse
[05:22:09] <jmkasunich> now they are handled in replace_vars, but still incorrectly (# in a quoted string still truncates)
[05:22:42] <jmkasunich> I'm thinking that parse is definitely the wrong place
[05:23:02] <jmkasunich> replace_vars doesn't seem like the right place either
[05:23:07] <SWPLinux> hmmm - replace_vars ignores quotes entirely
[05:23:16] <jmkasunich> right
[05:23:22] <SWPLinux> they're not in the word character list though
[05:23:28] <SWPLinux> words
[05:24:03] <jmkasunich> words is really the list of chars that are acceptable as part of a env or ini file variable, right?
[05:24:10] <SWPLinux> yes
[05:24:49] <jmkasunich> I've always like the approach used in tokenize for this kind of processing
[05:24:58] <jmkasunich> its a state machine
[05:25:17] <SWPLinux> yep. it is simple
[05:25:32] <jmkasunich> it could easily be processed to handle comments
[05:25:41] <jmkasunich> but would ignoring comments in replace_vars be a problem?
[05:25:51] <jmkasunich> you might do replacements inside comment text
[05:26:05] <SWPLinux> I don't like that idea, actually
[05:26:22] <SWPLinux> if it isn't semantically part of the command, it should be left alone
[05:26:52] <jmkasunich> which idea don't you like?
[05:27:05] <SWPLinux> doing replacements inside comments
[05:27:06] <jmkasunich> I don't like the idea of replacing vars inside comments
[05:27:08] <jmkasunich> ok
[05:27:37] <SWPLinux> replace_vars should look at the part of the string that means something to halcmd
[05:27:42] <jmkasunich> since tokenize is already aware of quotes, it can easily deal with comments
[05:28:01] <jmkasunich> would it make sense to tokenize (and strip comments) first, and then do variable replacement?
[05:28:11] <SWPLinux> well, you could certainly stick the replacement stuff inside tokenize()
[05:28:19] <SWPLinux> but I think it's not there for a reason
[05:28:39] <jmkasunich> actually, you can't stick replacement inside tokenise without a major rewrite
[05:28:54] <SWPLinux> sure you can - you're just inside a different kind of token if it's a var
[05:28:56] <jmkasunich> tokenize operates on the original string by breaking it up, but you can't do anything that changes the overall length
[05:29:11] <SWPLinux> more states, and a little work to do when the end of the token (closing paren, etc) is found
[05:29:34] <SWPLinux> true, but that's not a problem, since we can allocate additional buffer(s)
[05:29:54] <jmkasunich> understood - but that is a non-trivial change to tokenize
[05:30:21] <SWPLinux> hmmm. actually, the token array can just be dynamically allocated. that would make some things simpler
[05:30:22] <jmkasunich> I'm trying to think of a reason we can't do replacement after tokenization
[05:30:44] <SWPLinux> as long as the tokenizer knows about parentheses, it should be OK
[05:30:53] <SWPLinux> I think it's acceptable to have a shell var with a space in the name
[05:31:08] <jmkasunich> in the name? are you sure
[05:31:30] <jmkasunich> no, names can't have spaces
[05:31:35] <SWPLinux> hmmm
[05:32:05] <jmkasunich> jmkasunich@ke-main-1006:~/emcdev/emc2head$ foo=bar
[05:32:06] <jmkasunich> jmkasunich@ke-main-1006:~/emcdev/emc2head$ echo $foo
[05:32:06] <jmkasunich> bar
[05:32:06] <jmkasunich> jmkasunich@ke-main-1006:~/emcdev/emc2head$ foo bar=blat
[05:32:06] <jmkasunich> bash: foo: command not found
[05:32:06] <jmkasunich> jmkasunich@ke-main-1006:~/emcdev/emc2head$ echo $foo bar
[05:32:09] <jmkasunich> bar bar
[05:32:34] <jmkasunich> the _value_ of the variable can certainly have spaces
[05:33:01] <jmkasunich> doing replacement before tokenization allows one variable to produce multiple tokens, doing it after does not
[05:33:08] <jmkasunich> dunno if that matters
[05:33:47] <SWPLinux> that could matter
[05:33:56] <jmkasunich> yeah
[05:34:08] <SWPLinux> you can set something to "show parport" ...
[05:34:19] <jmkasunich> seems like we need: comment strip, replacement, tokenize, parse
[05:34:24] <SWPLinux> yep
[05:34:25] <jmkasunich> in that order
[05:34:39] <SWPLinux> comment strip is easy, if you ignore quotes ;)
[05:34:41] <jmkasunich> comment strip will duplicate some tokenize code (quoted strings) but so what
[05:34:52] <jmkasunich> can't ignore quotes
[05:34:55] <jmkasunich> shouldn't anyway
[05:34:57] <SWPLinux> comment / whitespace strip, actually
[05:35:19] <jmkasunich> tokenize uses whitespace to separate tokens
[05:35:34] <SWPLinux> I meant strip off the ends
[05:35:50] <SWPLinux> so " foo bar " is the same as "foo bar"
[05:35:59] <jmkasunich> tokenize deals with that fine
[05:36:10] <jmkasunich> as long as replace_vars does too, I'd not mess with it
[05:36:35] <jmkasunich> I'm thinking that comment_strip() would work with the string in place, just like tokenize()
[05:37:02] <jmkasunich> all it needs to do is keep track of whether its inside a string, if not, replace # with \0 and return
[05:38:42] <jmkasunich> in fact, it could replace the entire comment with " " instead, that would allow (dunno why we'd care) comments that end, like C's /* */ style
[05:38:44] <SWPLinux> that's an annoyingly hard task to do with any method other than scanning through the string manually
[05:38:57] <jmkasunich> what is? remove_comments()?
[05:39:04] <jmkasunich> trivial with a state machien
[05:39:15] <SWPLinux> no - looking for comments but not inside quotes ...
[05:39:18] <jmkasunich> and yes, it does loop thru the string, but who cares
[05:39:30] <jmkasunich> the code would be shorter than tokenize
[05:39:31] <SWPLinux> yeah - trivial with a state machine, difficult with "bulk" commands
[05:39:41] <jmkasunich> who cares about bulk commands
[05:39:46] <SWPLinux> just annoying, that's all :)
[05:40:19] <jmkasunich> sure, their loops might be a bit faster, but the minute you run strlen twice, its probably slower then writing your own loop with the logic embedded in the loop
[05:40:42] <SWPLinux> only if gcc is extremely smart about optimizing loops
[05:40:44] <jmkasunich> and IMO the state machine is easier to understand
[05:40:52] <SWPLinux> the libnrary commands use intrinsic REPT commands
[05:41:04] <SWPLinux> scasb, etc
[05:41:10] <jmkasunich> the fact that the state machine accesses each char only once is a win IMO
[05:41:34] <SWPLinux> well, that's fine. for some of this stuff, it does make it easier
[05:42:02] <SWPLinux> in fact, the whole thing can be collapsed into one function if it's OK to make a second command buffer
[05:42:29] <jmkasunich> yeah, but then the logic is harder to understand
[05:42:40] <SWPLinux> I don't think so. it's still just a state machine
[05:42:51] <SWPLinux> but you have states like IN_PAREN_ENV_VAR
[05:43:04] <SWPLinux> what to do in that state is pretty simple to understand
[05:43:26] <jmkasunich> matter of taste I guess
[05:43:32] <jmkasunich> I like the multi-stage approach
[05:43:55] <SWPLinux> I think I wanted replace_vars separate so it could be used elsewhere, but if it isn't needed that way (and it isn't currently used anywhere except in front of tokenize), then that's not important
[05:44:01] <jmkasunich> each stage is simpler, has fewer states, and fewer possible combinations
[05:44:10] <SWPLinux> that's true
[05:45:11] <jmkasunich> I'd suggest that we remove the comment chopping code from replace_vars, but keep it generic (as you say, we might want to use it elsewhere some day)
[05:45:13] <SWPLinux> though I think it's harder to maintain. if we add a <> token type, then everything that runs before the replacement has to know about it
[05:45:27] <jmkasunich> remove the comment chopping code from parse as well
[05:45:36] <jmkasunich> and add a comment stripping function
[05:45:59] <jmkasunich> the comment stripper is the only thing before replacement
[05:46:27] <jmkasunich> and unless you want to allow # inside a variable name or token (other than a quoted string) the stripper doesn't care
[05:46:28] <SWPLinux> ok. do you have an objection to having it also remove leading and trailing whitespace?
[05:46:46] <jmkasunich> no fundamental objection
[05:46:51] <SWPLinux> is # a valid HAL pin character?
[05:46:57] <jmkasunich> no
[05:46:59] <SWPLinux> ok
[05:47:01] <jmkasunich> its a comment
[05:47:31] <jmkasunich> if some driver decides to use # in a config string, then you have to quote the string
[05:47:32] <SWPLinux> if it's valid anywhere, then it should be recognized while inside any quoting pair (such as parens, quotes, or brackets)
[05:47:58] <jmkasunich> " ", ' ' are supported
[05:48:25] <jmkasunich> [ ]. ( ) are used to quote variable names only, they aren't allowed to have # in them
[05:49:09] <jmkasunich> if we wanted to be purists, we'd also support an escape character
[05:49:16] <SWPLinux> I ws just thinking about that
[05:49:17] <jmkasunich> \# would not start a comment
[05:49:28] <jmkasunich> \[ would not start a var substitution
[05:49:31] <jmkasunich> etc
[05:49:41] <SWPLinux> right - I was working through the issue of looking at the previous char, and what happens at the start of a line :)
[05:50:07] <jmkasunich> you don't look at the previous
[05:50:14] <jmkasunich> its just another state
[05:50:35] <SWPLinux> yes
[05:50:53] <jmkasunich> if ( ESCAPED ) then all_chars_treated_ as_ordinary; ESCAPED == 0;
[05:51:18] <jmkasunich> this is getting out of hand
[05:51:29] <jmkasunich> I don't want to rewrite the whole thing
[05:51:51] <jmkasunich> I want to fix the potential buffer overflows, and the incorrect comment processing
[05:52:03] <jmkasunich> maybe later (2.2) we should revisit and refactor the parsing code
[05:53:18] <SWPLinux> yep
[05:55:20] <SWPLinux> crap. 1AM again
[05:58:03] <jmkasunich> I just noticed that tokenize() and the comment stripper I'm writing right now don't handle single quoted strings nested inside double quoted ones, or vice versa ;-)
[05:58:13] <SWPLinux> heh
[05:58:15] <jmkasunich> that can come later, along with ecape chars
[05:58:29] <jmkasunich> I'll have a comment stripper done in 5 mins
[06:02:08] <jmkasunich> does this look right: http://www.pastebin.ca/316086
[06:04:10] <jmkasunich> as a bonus, it returns -1 if there is an unterminated quoted string
[06:04:30] <SWPLinux> unless it's inside the other kind of quotes ;)
[06:04:48] <jmkasunich> yeah, it doesn't handle nested quoted strings
[06:05:09] <jmkasunich> it would need a stack to do that, you could next them arbitrarly deep if you wanted to be a pain
[06:05:10] <SWPLinux> I think you need a stack for that
[06:05:14] <SWPLinux> :)
[06:05:58] <jmkasunich> anyway, I think I'm gonna stick this in before replace_vars(), and remove the comment processing from both replace_vars and parse_cmd
[06:06:13] <SWPLinux> ok. it looks right to me
[06:06:32] <jmkasunich> replace_vars still needs overflow-proofed
[06:06:53] <jmkasunich> we've been talking instead of coding, you still want to do that (tomorrow perhaps)?
[06:07:21] <SWPLinux> yep. can do, hopefully tomorrow
[06:07:25] <jmkasunich> ok
[06:07:38] <jmkasunich> I'll commit my changes so far before I go to sleep
[06:07:44] <SWPLinux> ok
[06:07:52] <jmkasunich> the only change inside that function will be removal of the # detection
[06:08:23] <SWPLinux> can you mark the overflow spots then? I'll update before changing anything
[06:09:01] <SWPLinux> (at least the ones you nioticed - I'll look for others as well)
[06:09:15] <jmkasunich> want me to put "fixme" in there?
[06:09:28] <jmkasunich> I can do that
[06:09:51] <SWPLinux> sure: FIXME: buffer overflow possible or similar
[06:09:59] <jmkasunich> will do
[06:10:03] <SWPLinux> thanks
[06:10:19] <SWPLinux> my memory is terrible these days, especially when things go in late at night
[06:10:33] <jmkasunich> halcmd has had a lot added to it lately, so a good review from top to bottom is probably a good thing
[06:10:48] <SWPLinux> yep
[07:21:44] <jmkasunich> ds3 is rather stubborn isn't he
[07:22:43] <SWPadnos> heh - seems that way
[15:25:32] <cradek_> cradek_ is now known as cradek
[16:47:59] <jepler> did you guys review "net"? or just parsing in general?
[16:48:11] <jepler> that reminds me that I should figure out WTH quoted arguments don't work with rtapi_app
[18:17:24] <alex_joni> jepler: around?
[18:17:56] <alex_joni> I just saw there are a few texts in AXIS which aren't translated.. any idea how to regenerate the .po with the new added strings?
[18:24:38] <jepler> alex_joni: something about "msgmerge"
[18:25:13] <jepler> alex_joni: I ended up taking the "msgmerge" out of the automatic build process for two reasons. first, edits to almost any source file would cause changes in all of the language .po files, even though no useful change was made
[18:25:24] <jepler> second, translators seemed to have preferences about the "msgmerge" arguments to be used
[18:25:41] <alex_joni> oh ok
[18:26:04] <jepler> I think the the basic commandline is: msgmerge ro_axis.po axis.pot
[18:26:08] <alex_joni> I just noticed the german translation is dated Aug 10
[18:26:37] <jepler> or maybe msgmerge -U
[18:26:49] <jepler> yeah that creates the new output in ro_axis.po instead of stdout
[18:26:53] <alex_joni> yeah
[18:26:55] <alex_joni> -U works
[18:27:43] <alex_joni> I think we should mail translators to get busy :)
[18:27:49] <alex_joni> release is coming soon
[19:04:23] <jmkasunich> jepler: I started out with the intent of specifically reviewing net, but I got sidetracked
[19:04:31] <jmkasunich> I'm gonna review as much of halcmd as I can
[19:05:12] <jepler> now you'll discover how much I've crapped it up over time :(
[19:05:24] <jepler> I'm glad you're doing it, though
[19:05:32] <jepler> thanks
[19:06:33] <jmkasunich> that file is kinda getting out of hand - almost 3900 lines
[19:06:44] <jmkasunich> maybe for 2.2 we should look at splitting it up
[19:08:25] <jmkasunich> just in what I've reviewed so far... we should probably be using getopt to parse command line options (yeah, I know I'm the one who didn't do that in the first place)
[19:08:37] <jmkasunich> about 2/3 of main is parsing options
[19:45:43] <jepler> the commandline completion stuff is a big pile of code as well
[19:48:31] <jmkasunich> yes, but isn't that pretty much the nature of the beast?
[19:48:44] <jmkasunich> best that could be done there is putting it in its own file I think
[19:48:50] <jepler> maybe so
[19:49:15] <jmkasunich> that would still greatly declutter halcmd.c
[22:19:55] <jmkasunich> I wish I knew why everybody and his brother thinks the way to an embedded EMC is stripping out a few source files.....
[22:20:14] <jmkasunich> when there are gigabytes of other crap on the disk that is far easier to get rid of
[22:20:19] <alex_joni> lol
[22:20:26] <alex_joni> I just had that question from roltek last night
[22:20:31] <jmkasunich> roltek was just /msg'ing me about it
[22:20:45] <alex_joni> in the end I convinced him to change the =m in Makefile.inc.in
[22:20:54] <alex_joni> that way unneeded drivers don't get built
[22:21:06] <alex_joni> but that's about all an unexperienced user can do :D
[22:21:13] <jmkasunich> the way he asked me, he actually wants to delete source file
[22:21:15] <jmkasunich> ds
[22:21:18] <jmkasunich> bah
[22:21:21] <jmkasunich> source files
[22:21:37] <alex_joni> tell him he's on his own on that crap
[22:21:44] <jmkasunich> I pretty much did
[22:22:18] <jmkasunich> I told him if he built on a regular system and copied over the binaries he needs it will eliminate _all_ the source
[22:22:20] <jmkasunich> he didn't care
[22:22:45] <alex_joni> I know
[22:22:51] <alex_joni> I had the same conversation last night
[22:22:53] <alex_joni> with roltek
[22:22:54] <alex_joni> :/
[22:31:28] <alex_joni> hmm.. halshow has a small bug in it.. if you maximize the window it sizes the scroll bars
[22:33:09] <alex_joni> http://imagebin.org/6914
[22:33:48] <jmkasunich> heh, don't you just love tcl/tk
[22:34:32] <alex_joni> obviously not enough to fix it
[22:36:16] <jepler> verry cute
[22:38:18] <alex_joni> is there a threading gcode in nc_files?
[22:38:30] <alex_joni> duh.. threading.ngc
[22:38:30] <jmkasunich> hmm.. some extreme low probability mutex issues in halcmd net
[22:38:54] <jmkasunich> it gets the mutex, then finds the first pin, releases the mutex, calls new_signal, gets the mutex, etc
[22:39:14] <jmkasunich> ideally the mutex would be held for the entire operation
[22:39:30] <alex_joni> lol, spindle speed override & axis-lathe & threading.ngc is so much fun
[22:39:34] <jmkasunich> starting before the preflight
[22:40:10] <jepler> jmkasunich: yes, but hal_link will acquire the mutex
[22:40:31] <jepler> otherwise it would be much simpler
[22:40:43] <jepler> halpr_link_with_lock_already_acquired() ?
[22:41:14] <jepler> halpr_link_with_a_name_shorter_than_the_one_already_proposed() ?
[22:42:06] <alex_joni> hal_pointer_link_with_a_literal_name_way_shorter_than_the_one_jeff_already_proposed_in_order_to_make_it_more_readable()
[22:48:29] <alex_joni> jepler: oh, you shouldn't have bothered.. but thanks
[22:49:14] <jepler> I shouldn't have? why not?
[22:49:20] <alex_joni> dunno :)
[22:49:27] <alex_joni> it wasn't a big deal
[22:49:56] <jmkasunich> jepler: I know the existing HAL api doesn't let you do it the "right" way
[22:50:03] <alex_joni> guess you don't feel like having halshow on the axis menu .. do you?
[22:50:10] <jepler> alex_joni: it's already there
[22:50:22] <alex_joni> didn't see it.. only halmeter & halscope
[22:50:25] <jepler> Machine > Show HAL Configuration
[22:50:28] <jmkasunich> and the fact that we don't normally run multiple halcmds at the same time means it doesn't really matter
[22:50:37] <alex_joni> oh, there..
[22:51:14] <jmkasunich> its funny, halcmd was meant to be a thin layer over the HAL api
[22:51:34] <jmkasunich> but as we've extended the capabilties of hal, we've done it in halcmd
[22:54:54] <jepler> is 'iosh' used at all these days?
[22:55:11] <jmkasunich> I don't think so, but can't be sure
[22:55:27] <alex_joni> I bet not
[22:55:36] <alex_joni> I removed some stuff from it, and no-one complained
[22:55:55] <alex_joni> when I moved the spindle stuff to motion...
[22:56:07] <alex_joni> iosh only talks to the iocontroller, not to task/motion..
[22:57:19] <alex_joni> hmm.. I'm getting 1 warning when starting xemc
[22:57:27] <alex_joni> Warning: Cannot convert string "-adobe-courier-bold-r-normal--64-*-*-*-*-*-*-1" to type FontStruct
[22:57:44] <alex_joni> xemc does start up, but with a tiny font
[23:23:23] <cradek> hi guys
[23:23:37] <alex_joni> hi chris..
[23:24:02] <cradek> finally some time to talk
[23:24:34] <alex_joni> heh.. busy day?
[23:25:12] <cradek> yes, some things to do, but home now
[23:41:43] <cradek> so how's emc 2.1?
[23:42:24] <cradek> looks like just a few backports are still pending
[23:42:43] <alex_joni> I think it's fine
[23:42:52] <alex_joni> been stresstesting it here, lots of configs & all
[23:42:56] <alex_joni> looks just fine to me
[23:43:33] <cradek> did you take PROBE_* out of the inis (and check for other bogus things?)
[23:44:01] <alex_joni> I found a couple of minor things.. but I'll leave them for another time (xemc has problems with the default readout font; tkemc & xemc have problems locating their help files)
[23:44:10] <alex_joni> no, not yet
[23:44:22] <alex_joni> ahh.. remembered what I wanted to ask you :)
[23:44:38] <alex_joni> can you add a README for nist-lathe and for lathe-pluto ?
[23:45:06] <cradek> the help path is in the inis... I changed all of them manually for the 2.0 series
[23:45:18] <cradek> for the packages I mean
[23:45:25] <alex_joni> yeah, we need to do the same here
[23:45:54] <alex_joni> but in the end (HEAD) I'll make it set the path from the runscript, and only the filename in the ini
[23:46:50] <cradek> a while back I worked on updating that help file a bit - it might still be very out of date - I don't remember for sure
[23:47:19] <alex_joni> yes.. maybe integrating the html help might be better
[23:47:50] <alex_joni> but that's certainly 2.2 or later :)