Re: [Toybox] [PATCH 0/1] Added basic post-data support to wget.c
On Sat, Mar 26, 2022 at 2:49 PM Rob Landley wrote: > On 3/26/22 11:59 AM, Moritz C. Weber wrote: > > Dear all, > > I wrote the simple patch to enable wget for POST requests with an > --post-data option. > > Sadly, patched wget and git HEAD segfault on my current build machine. > > I did some cleanups to wget recently (after talking to you about it), and > applied your patch on top of those. > > You got the argument order backwards in GLOBALS, the arguments from right > to > left in the NEWTOY() string populate the structure from top to bottom. I > need to > finish the tutorial video on that... > or reverse them in the code? this always gets me too. is it easier to just fix it (by having a max that we subtract from?) so struct fields can be in the "obvious" order, rather than add more explanations of why it's "backwards" (for non-RTL language natives at least)? it would be a disruptive diff to say the least, but better sooner than later? > I admit it's a little nonobvious. The bits go from right to left, > NEWTOY(blah, "abcd", FLAGS) means d is (1<<0), c is (1<<1), b is (1<<2) > and so > on. So "blah -a -b -d" would set toys.optflags to binary 1101. > > GLOBALS() is essentially treated as a union of the command's type > structure, and > an array of long that gets populated in packed bit order. So "a#b*cd:" > would be: > > GLOBALS( > char *d; > struct arg_list *b; > long a; > ) > > Since "-c" doesn't take an argument, it doesn't consume a slot. > > Linux is LP64 so sizeof(pointer) == sizeof(long), and register sized > entries in > structures have no packing between or before them, so -d arg goes in TT.d, > -b > arg goes in TT.b, and -a arg goes it TT.a. > > Note that when they're the same type I collate the declarations, and C > declarations go "left to right then top to bottom". So: > > NEWTOY(blah, "a#b:cd:", FLAGS) > GLOBALS( > char *d, *b; > long a; > ) > > NEWTOY(blah, "a#b#cd:", FLAGS) > GLOBALS( > char *d; > long b, a; > ) > > (By convention I put a blank line between variables populated by arguments > and > globals that are just used by the code to store stuff. The latter are > always > initialized to zero.) > > > So, testing can and will be improved, but I mainly send this patch to > learn > > the process. And I think the change is trivial, if I got everything > right. > > > > I am not used to write C and I am still learning the toybox > infrastructure. > > So I would be happy to get feedback what to do better. > > sizeof(pointer) gives you the size of the pointer variable (4 bytes on 32 > bit > systems, 8 bytes on 64 bit systems). You wanted strlen(pointer). I fixed > it up > already. > > The other thing is an issue that was already there in wget (and part of the > reason it's still in pending): toybuf is a 4k buffer but there's no limit > on the > length of the input arguments, so sprintf(toybuf, xxx) can run off the end > of > the buffer. I need to add length checks. (Working on it...) > > > The long-term purpose for this patch would be to refactor the wget logic > into > > the library, so that I could reuse GET and POST in a git toy, which I > plan. > > That is good to do. The cleanups I'm doing should help towards that. > > Thanks, > > Rob > ___ > Toybox mailing list > Toybox@lists.landley.net > http://lists.landley.net/listinfo.cgi/toybox-landley.net > ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] [PATCH 0/1] Added basic post-data support to wget.c
On 3/26/22 11:59 AM, Moritz C. Weber wrote: > Dear all, > I wrote the simple patch to enable wget for POST requests with an --post-data > option. > Sadly, patched wget and git HEAD segfault on my current build machine. I did some cleanups to wget recently (after talking to you about it), and applied your patch on top of those. You got the argument order backwards in GLOBALS, the arguments from right to left in the NEWTOY() string populate the structure from top to bottom. I need to finish the tutorial video on that... I admit it's a little nonobvious. The bits go from right to left, NEWTOY(blah, "abcd", FLAGS) means d is (1<<0), c is (1<<1), b is (1<<2) and so on. So "blah -a -b -d" would set toys.optflags to binary 1101. GLOBALS() is essentially treated as a union of the command's type structure, and an array of long that gets populated in packed bit order. So "a#b*cd:" would be: GLOBALS( char *d; struct arg_list *b; long a; ) Since "-c" doesn't take an argument, it doesn't consume a slot. Linux is LP64 so sizeof(pointer) == sizeof(long), and register sized entries in structures have no packing between or before them, so -d arg goes in TT.d, -b arg goes in TT.b, and -a arg goes it TT.a. Note that when they're the same type I collate the declarations, and C declarations go "left to right then top to bottom". So: NEWTOY(blah, "a#b:cd:", FLAGS) GLOBALS( char *d, *b; long a; ) NEWTOY(blah, "a#b#cd:", FLAGS) GLOBALS( char *d; long b, a; ) (By convention I put a blank line between variables populated by arguments and globals that are just used by the code to store stuff. The latter are always initialized to zero.) > So, testing can and will be improved, but I mainly send this patch to learn > the process. And I think the change is trivial, if I got everything right. > > I am not used to write C and I am still learning the toybox infrastructure. > So I would be happy to get feedback what to do better. sizeof(pointer) gives you the size of the pointer variable (4 bytes on 32 bit systems, 8 bytes on 64 bit systems). You wanted strlen(pointer). I fixed it up already. The other thing is an issue that was already there in wget (and part of the reason it's still in pending): toybuf is a 4k buffer but there's no limit on the length of the input arguments, so sprintf(toybuf, xxx) can run off the end of the buffer. I need to add length checks. (Working on it...) > The long-term purpose for this patch would be to refactor the wget logic into > the library, so that I could reuse GET and POST in a git toy, which I plan. That is good to do. The cleanups I'm doing should help towards that. Thanks, Rob ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
[Toybox] [PATCH 0/1] Added basic post-data support to wget.c
Dear all, I wrote the simple patch to enable wget for POST requests with an --post-data option. Sadly, patched wget and git HEAD segfault on my current build machine. So, testing can and will be improved, but I mainly send this patch to learn the process. And I think the change is trivial, if I got everything right. I am not used to write C and I am still learning the toybox infrastructure. So I would be happy to get feedback what to do better. The long-term purpose for this patch would be to refactor the wget logic into the library, so that I could reuse GET and POST in a git toy, which I plan. Best regards, Moritz Moritz C. Weber (1): Added basic post-data support toys/pending/wget.c | 21 +++-- 1 file changed, 15 insertions(+), 6 deletions(-) -- 2.20.1 ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
