On Wed, Jan 13, 2010 at 4:10 PM, Jeff Garzik <jeff@xxxxxxxxxx> wrote: > OK, final review. Please fix these problems and resend (some of these > repeated from other emails; putting here to consolidate): > > * needs to build patch builds and runs against a fresh git repo clone + > ./autogen.sh + ./configure + make distcheck... Arg! Looks like it needs more work in the automake department. FWIW (which isn't much), ./autogen + ./configure + make did work for me. I probably had some previous build products hanging around in the system include paths. Also, your point about "make clean" is well taken. > * you increased the size of struct cldc_call_opts to over 256k! That is > unworkable in multi-thread programs, where struct cldc_call_opts may be > allocated on the stack -- as it is in most of the tool and test code using > libcldc. another solution, acceptable to usage within a thread (often > limited to 8k stacks) must be found. Good point. It could be solved by dynamically allocating the buffer, or by moving the statically allocated temporary buffer into cldc_session. Dynamic allocation is probably better, but it will require API users to free the buffer after use. > * it would be nice if this patch were separated out a bit, as there are a > lot of semi-related changes and cleanups mixed in amongst the XDR > conversion. I'm not 100% sure how much work this would be. I can take a look, at least... > * do we really need separate pkt_is_first() and pkt_is_last() ? I tend to > prefer a flags-based approach, where the code tests bits in a 'flags' > variable. I'll make it flag-based. > > * the stuff in cld_fmt.h is essentially global application identifier > namespace. as such, exporting functions like 'dump_buf' or 'opstr' may run > into trouble. suggest '__cld_' prefix, perhaps. Yeah, cld_fmt.c functions get linked into libcldc, so it's best to prefix them with "cldc_" sincerely, Colin -- To unsubscribe from this list: send the line "unsubscribe hail-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
