On Fri, Mar 20, 2015 at 10:04:19AM +0100, Laurent Bercot wrote:
>
> >Well, I've poked at this a little this evening.
> >
> >See
> >https://github.com/idunham/busybox
> >(in branch mdev) if you're curious about details.
>
> Hi Isaac,
> Nice job!
Thanks.
> A few comments:
>
> - Original mdev relies on getenv/putenv to handle environment
> variables; that's okay, because it's a short-lived program.
> But mdev -i is a long-lived program, so using and modifying its
> own environment for every event is conceptually unsatisfying, and
> ends up in a lot of unnecessary malloc/free/reallocs. It would be
> faster and more memory-efficient to allow handle_event to work with
> the raw string containing the VAR=VALUE\0 pairs.
While putenv/clearenv seems rather lame, I've been trying to keep the
modifications to a minimum.
Again, I haven't tested this yet - I don't think it's time to deal
with those until it works.
I'm not sure I follow you on the malloc/free/realloc bit:
-as far as I know, getenv() does not malloc() anything, but returns
the numeric value of a pointer within char ** environ
-according to the manpage, putenv() does not copy the string, but
modifies environ by inserting a pointer to the string.
If the environ buffer is too short, environ may be expanded by
calling realloc() - is this what you refer to?
-setenv() and unsetenv() call malloc/free, but I explicitly avoided them
-I remember reading that clearenv() does call free() on Solaris, but I'm
not aware of any Linux libc that does so; also, there is no dynamically
allocated memory to free(), since I'm using pointers into msgbuf.
> I realize it may require modifying all the getenv() invocations in
> the rest of mdev, but I believe it's worth it. (Heavy getenv()
> usage in mdev was one of the parts that made me postpone my plans of
> modifying it myself...)
env_matches(), make_device(), and handle_event() are the only
functions that need to have it replaced, and the first two have
one use each.
handle_event() has a few cases of it.
I'm thinking that it may make sense to use
getkey(char *key, char *buf, size_t n);
in places that use
getenv(key);
buf is a buffer containing an event stored according to your protocol
and n is the size of the buffer.
It would be possible to make this a wrapper around getenv() if
buf is NULL or n is 0.
We could make even make mdev -s work right by reading the uevent
file into a buffer, replacing newlines with \0, and using that
instead of the old code.
But this is long-term stuff (should happen before merging, but
this needs to work before that happens.)
> - Why do you need to memset practically the whole buffer every
> poll() iteration ? You know where you read and how many bytes you
> read, it should be possible to be unaffected by irrelevant data in
> the part of the buffer you're not using.
I should probably replace the memset() before read() with
msgbuf[len] = 0; after read().
I was just using a hammer to deal with strlen possibly overrunning
the buffer, should we ever read something that ends with a partial
KEY=VAL pair and no NUL terminator.
> (It would also be nicer to have real circular buffer management,
> but that's a lot of code, with struct iovecs and such; not sure if
> it's worth, considering very few busybox applets would benefit from
> it.)
If that's desired, someone else will need to do it.
I never formally studied C, just read bits of a few old books on it
(Using Turbo C, Learning to Program in C, The C Language, a short
skim of Writing Portable C) and fixed programs that didn't compile
or crashed.
So while I know that you're referring to writev(), I don't know
how to use it.
> Thanks for getting to the dirty work, I really appreciate it!
I waited as long as I did in hope that someone else would pick it up.
> --
> Laurent
Thanks,
Isaac
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
