<dddddd>fossy, about struct_to_array (which might need a better name, related to the linked-list instead of about struct). What's the meaning of that 4096 magic number and how this tell apart tokens from environment (what forces that rule/invariant)? -- It seems to me that array[i] doesn't need any allocation as you are overwriting the pointer no matter what. -- The "line too long" error message seems incorrect, as the check is about the max <dddddd>_number_ of args IIUC the MAX_ARGS constant. <fossy>4096? is what oriansj had always used <fossy>MAX_ARGS also happened to be used for arrays <fossy>It should be Sscript too long <dddddd>The is_envar caller keeps checking for 1 instead of the TRUE constant. <dddddd>Has that 4096 any relation with MAX_STRING? <dddddd>--strict is not listed as possible option in the help. And I'm a bit confused about the expected behaviour when it's set, so maybe I'll ask about it later on. <dddddd>collect_variable is documented as "to substitute variables", weird function name isn't it? <fossy>4096 is what max string is defined as <dddddd>As for the escaping behaviour, what do you think about printing at least a warning (if not error) if a non-supported char is found (as it's only "supporting" newlines)? <dddddd>Yeah, MAX_STRING is 4096 but does have it any relation with the other 4096 or it's just coincidence? <fossy>I dont recall lemme look in a second <dddddd>No hurry for any answer, take your time whenever you're free. <fossy>The escaping behaviour - do you think that it warrants a warning? <dddddd>I do think so, because we're dropping input on the floor. <dddddd>An error wouldn't be crazy even, I think. <dddddd>We'll what others think about it... <dddddd>So, set -e shouldn't cover builtins, right? <OriansJ>well the \ for EOL escape certainly merits a warning when people do say \n and see nothing <xentrac>if a builtin like [ -f $x ] fails, the script should bail out <xentrac>what prevents it from bailing out in if [ -f $x ]; then foo; else bar; fi is not that what's failing is a builtin <xentrac>what prevents it from bailing out is that it's in the condition of an if <dddddd>For reference... set, cd, pwd and echo are the supported builtins. <xentrac>usually if someone says set -e, it is essential for cd failing to abort the script <xentrac>early versions of sh actually would abort the script if cd failed even without -e being set, if memory serves <OriansJ>well it certainly would be easy for the builtins to get conditional error catching behavior <Hagfish>so giving one company control over millions of other people's computers didn't work out so well after all <dddddd>fossy, I think variables added in a line like "FOO=bar cmd" should not survive the execution of cmd but AFAIU they do in the current implementation. Is this by design, a bug or I'm missing something? <dddddd>Well, I guess we're not even executing cmd. <dddddd>hmm, maybe we're but as it were part of another line. I'm still not familiar enough with the codebase, so maybe I'm wrong. <dddddd>The so-called nightmare mode is not implemented. <dddddd>OriansJ, why is it called like that? <OriansJ>dddddd: because it kills the environment and alot of applications freakout when envp is just a NULL <OriansJ>Hagfish: I think it is the best news of the month; libreboot will be able to be on ALL modern Intel systems <OriansJ>dddddd: as I understand it variable support in kaem is quite minimal <dddddd>Kind of clever naming, but I guess we don't really need to be that fancy. What about --no-env or something like that, more semantic? <OriansJ>well dddddd I guess a more accurate name would be --init-mode <OriansJ>as no environment exists when spawned as init <dddddd>Do you think is worth renaming? Would it cause much breakage? <dddddd>fossy, also note that the short option for that mode lacks hyphen (I don't know it that's on purpose but it's not consistent with the rest of the options). <OriansJ>nothing is currently using --nightmare-mode as it is only useful should one wish to use it build a proper init on a posix without one but kaem-optional-seed could fill that role <OriansJ>which being only 737bytes would probably be a better bootstrap init <dddddd>Last question for today fossy. What's in Token->pos? The position of the last char in the line? Is it meaninful at all or is it just a temporal value instead? I guess that'd be interesting to document. <dddddd>Also explaning which part of the union is used for each kind of token. If we're going to reuse the struct, at least that the behaviour is clear. <fossy><dddddd> fossy, I think variables added in a line like "FOO=bar cmd" should not survive the execution of cmd but AFAIU they do in the current implementation. Is this by design, a bug or I'm missing something? <fossy>dddddd: token->pos is used for a variety of things <fossy>mostly tracking the posiiton between functions <fossy>it resets after each "stage" <fossy>so ie, token collection, variable and exec <fossy>thanks for all the comments ^-^ <Hagfish>the discussion on HN about the intel thing suggested that the lack of support for libreboot (or coreboot) is more to do with binary blobs than signing <Hagfish>i didn't understand the details, but i'm sure that opening up the chip to modification must be useful for firmware projects <xentrac>seems likely, but there's probably a lot of work to do between here and there <fossy>OriansJ: how would n->var == NULL occur? Faulty code in other parts of the program. <fossy>i agree that masking it is not a solution, but making sure end users do not encounter it is important imo ***ChanServ sets mode: +o rekado
<fossy>i will be around tomorrow morning, during when you guys are mostly active <fossy>i would really like to try to get this out the door this weekend <OriansJ>fossy: then put a require in and we can root out those faulty bits; doing the correct thing is the most important first step. ***janneke_ is now known as janneke
<fossy>OriansJ: I did fix the underlying bug <fossy>but I left the require there <fossy>would it be preferable for the require to be removed, now that the underlying bug is fixed? <dddddd>hi, fossy. I don't understand how using 4096 instead of the MAX_STRING constant is "explicit". The point a tried to make is that... if that number is 4096 because the logic (I see the reasoning now in the new nice comment) is about the lenght of the strings, then using the constant is the right thing. That way, if the constant changes (an the strings might be longer) the logic is intact. <dddddd>Do you keep a list of the unaddressed points from the last conversations? <fossy>Hm, it shouldn't be destructive... <dddddd>that might explaing why some things are unchanged. <dddddd>I read you want to finish the current work in kaem this weekend. How far do you want to go? I'm glad to help if you want and time slots match. <fossy>dddddd: Not any new features or anything, just get reviews of the code ive been sending done and the new kaem repo pushed <dddddd>Do you consider extending "set -e" to the builtins? <dddddd>We were talking about that builtins should catch errors to honor "stop on error" <fossy>That is probably a valid idea <dddddd>nice, another option is to put a TODO note. Ack the limitation but document it. Your choice... <fossy>dddddd: is there anything else in the code, that warrants attention? <fossy>if not, ill wait for oriansj, and then finish fixing the repository and push <dddddd>there're lots of thing, on a wide range of importance from my limited point of view <fossy>dddddd: do you think memory allocation errors in builtins should pass silent under set -e <fossy>dddddd: any of them that you think are important <dddddd>No, allocation error means broken behaviour. The program should stop. <fossy>Could be as simple as OOM, not broken behaviour <dddddd>If you can't allocate memory for your data, you can store your data, you can't do the task at hand. <dddddd>About the really important changes, I'll defer to OriansJ which has more experience. From then on... we can clean as much as you want. <dddddd>Maybe starting with the user visible interface. The usage help is not right, for example. <dddddd>About the new warnings feature, is it off by default for compatibility reasons or anything else? <fossy>Any existing scripts that may "use" the bug will get warnings spewed out <fossy>My idea was scripts should be developed and tested with -w, but shouldn't be run with them <dddddd>Remember --init-mode instead of --no-env. And the bug about the without-hypen check. <dddddd>Next on a fast list would be not allocating memory if we're not going to use it. <fossy>I guess I would approach that by going through each calloc and optimising it <fossy>Usage: ./kaem [-h | --help] [-V | --version] [--file filename | -f filename] [--verbose] [--no-env] [--fuzz] <dddddd>Yeah, this thingss need systematic methods. <fossy>dddddd: Should -h include descriptions of options? <dddddd>and the thing about --init-mode. Please, try to be methodic. <fossy>I try to be methodic... but I easily look over things <fossy>Hmm, I feel like I need to seperate some of these changes, like the --init-mode thing, into new commits -- they're not really part of a refactor <fossy>I guess that's what I want to get done this weekend, finish the refactor commit <dddddd>We can document the options in the manpage or README. More info from -h would be nice but not a high priority if we have the file. I guess; but yeah, might be out of scope. <fossy>dddddd: as a newcomer to the kaem codebase, how do you think it is looking in terms of readability, and are the comments helping? <dddddd>Comments are definitely helping a lot. <fossy>OriansJ: do you feel kaem is up to scratch and of the appropriate quality to be pushed <dddddd>It's mostly straightforward, with some rough/convoluted edges here and there. The ->pos life is not obvious and I'm a bit mystified due to the "ifset" term. <dddddd>On minor details, I don't quite like those shouting error messages, and the hard thing... <dddddd>The high-level overview comment is maybe misplaced, as it it's in a function instead of on top level (overview). <dddddd>How would you describe the refactor in a couple of lines? What motivated it, what's improved, etc... <dddddd>For next/last round of review, if you're planning on reducing the scope of changes (and center on the refactor) a version matching that would be required. There's no point in reviewing more than the planned commits. <dddddd>Do you have an stimation of how much time you can allocate during the weekend to finish the task? <dddddd>And, more importantly. Are you happy with the results? <dddddd>Oh, that note about "Super strange stuff when referenceing envp", is a bit non-descriptive. What problem are you trying to workaround? <fossy><dddddd> On minor details, I don't quite like those shouting error messages, and the hard thing... <fossy>but OriansJ has had them from the start. <fossy>description? The code was not very intuitive. The data structures being used were not extremely appropriate and in the end were going to make it very difficult to further extend kaem. Functions were too large and too many code paths in each function (ie too much depth) <fossy>dddddd: when referencing envp[i] the output seemed to be all jumbled <dddddd>Oh, now it's far from that description, nice! <fossy>dddddd: How would you feel about renaming the commit from refactor to refactor and improve <fossy>the changes are all rather linked <fossy>So splitting them is proving painful <fossy>All I can see is mess in doing it <fossy>ifset is because I am substituting if the variable is set.... <dddddd>So, "performing an ifset" is checking if the variable is set? <dddddd>Yeah, I guess we're late for incremental changes and a single commit is expected. Describe it as you feel better. <dddddd>What's the case for not checking it (not doing an "ifset") in a function called *_ifset? <dddddd>prematch (unused) it's still there. Let me be honest. It doesn't give any confidence to see that past comments are skipped like that. <dddddd>--warn is not listed in the help message. <dddddd>I'll wait OriansJ comments and consecuent fixes. Then we can try to polish some finicky details. <fossy>When was prematch addressed? <fossy>I try to address everything, but I guarantee ignoring a review comment would never be done on purpose <fossy>dddddd: OriansJ told me that he would prefer the check for ifset to occur inside the ifset function <dddddd>right before you explained me that --strict == set -e <OriansJ>fossy: The Super strange occured on 6470753cbd425569edbe13c07cb80566760ff5da in mescc-tools <dddddd>Sure I know it's not on purpose, don't get me wrong. But it happend more than once (again, method). <fossy>OriansJ: Super strange what? <fossy>dddddd: This will become better over time as I refine my processes over time <dddddd>No need to sorry, fossy. Exactly, learning from mistakes. <OriansJ>The idea is envp becomes a list with 2 pointers to char* the variable and its value. The arguments just become a list which variable replacement occurs upon prior to execution. <OriansJ>with both lists becoming arrays just prior to execution <fossy>Which part of that does not currently occur? <OriansJ>fossy: well the variable replacement only occurs once instead of everytime <fossy>dddddd: I also am not very used to doing review by IRC, ive almost always done it via github/gitea, so it will take me some getting used to <fossy>OriansJ: Sorry, I still dont follow... when are you saying the variable replacement should occur? <OriansJ>fossy: if a string has 4+ variables in it all 4+ should be replaced <fossy>As I said earlier, that is not a goal in this refactor <fossy>But, variable support is currently minimal, and I would prefer to address that in the near future, but not right now <fossy>I will put that in a TODO comment, then <OriansJ>but it should be on your future TODO list as it will trip up people <OriansJ>collect_token, shouldn't allocate a token until the end <fossy>Maybe, a CAVEATS file is in order? <OriansJ>fossy: no, correctness is always the first goal <OriansJ>postpend_char should never be in loops <fossy>But I mean for things like this <fossy>Where we only currently support one variable <fossy>oh sorry I was referencing variables not postpend char <OriansJ>I'd rather have no variable support than wrong variable support <fossy>I will do that before push, removing all postpend_char from loops <fossy>OriansJ: so you would prefer no variables substituted rather then one variable substituted? <OriansJ>fossy: I'd rather we did variable substitution correct <dddddd>+1 no broken features. Even more, if it's really a refactor there should be not changes in behavior at all (but I guess that's the "improvements" added part) <fossy>do you feel that multiple variables in a token is important enough to block the pushing of this until I implement that? If so thats fine <OriansJ>but pushing half-broken pieces just isn't a good idea <fossy>1. Remove postpend char from loops <fossy>2. Make variable substitution support multiple variables <OriansJ>3. Remove any functions that are not used. <dddddd>Is postpend_char-in-loop a bug or just a question of speed? <fossy>dddddd: I think its for memory allocation reasons <fossy>postpend_char makes a calloc each time, iirc <OriansJ>postpend_char allocates a max_string on every iteration and writes the whole string everytime <OriansJ>So the memory becomes O(M*n) and processing becomes O(n^2) <fossy>how about prepend_string? same issue? <OriansJ>prepend_string does not appear to be in any of your loops fossy <fossy>No it isnt, just out of interest <OriansJ>but it should only be used to combine 2 completed strings and only in a loop if you are combining alot of completed strings together