[02:12] * Keybuk finally gets around to proving some of the bugs sadmac fixed in 0.5 [02:49] Keybuk: which ones? [02:52] $ENV's replacement being the same length as the variable name+$ for example [02:53] oh yes [02:53] I'm going to be strict from now on ;) all bug fixes must come with test cases [02:54] not a problem [02:54] * sadmac goes back to writing tests [02:58] Keybuk: test_com.netsplit.Nih.Test_object: tests/com.netsplit.Nih.Test_impl.c:766: my_setup: Assertion `server != ((void *)0)' failed. [02:58] Keybuk: I started getting these failures writing my testcases, but reverting my changes doesn't fix them [02:58] err [02:58] that implies your test d-bus server isn't running? [02:58] have you checked it got killed ok before? [02:58] usually it's test_dbus-daemon [02:59] I see a test_com.netspli [02:59] that's probably it [03:00] ahh, segmentation fault [03:00] something strangely comforting about knowing its your own fault after one of those phantom errors. [03:05] Keybuk: how much later will you be around? [03:06] not much [03:06] why? [03:07] Keybuk: I have the async api and one test case. I want to show it to you before I write more so I know if the API as at least right. But I have a segfault left to scrub out so its not quite ready :) [03:11] cool [03:11] I may be around long enough ;) [03:12] Keybuk: dbus_shutdown is triggering the segfault. Its being called from the teardown function (which I haven't touched). Only thing I can think of that would do it is a call to dbus_connection_read_write_dispatch (conn, -1); [03:12] seems like without that it didn't segfault (though the test didn't really run) [03:23] that implies you've got your references mixed up somewhere [03:24] ah [03:24] what's happening then? [03:25] * sadmac sees definite double-deref [03:26] dunno how that could do this, but its a bug [03:26] Keybuk: \o/ [03:27] Keybuk: how do you want this then? [03:35] got a bzr tree? [03:37] Keybuk: yeah, writing the commit notes now... [03:37] I had more outstanding than I thought. [03:46] Keybuk: https://code.launchpad.net/~cjdahlin/libnih/async [03:50] ok, give me a few minutes to finish up this bit [03:50] and I'll take a look [03:52] k [04:08] urgh [04:08] the valgrind attacks. you die [04:09] (not your code, mine) [04:09] http://rafb.net/p/EzdMTh82.html [04:10] nothing worse than an invalid write into a freed block where the write and free are inside the same function [04:10] (and that function is "free" :p) [04:10] ot [04:10] *oy [04:16] isn't it 4am over there? [04:22] yes [04:22] don't you ever stay up all night hacking? :) [04:25] why names? [04:28] unless I'm missing something, the dispatch function doesn't take the method call arguments [04:31] I'd generate a typedef for the callback function and put it in the .h file [04:31] and use that [04:31] if callback is NULL, should send without expected reply [04:32] NihAsyncNotifyData should be in the .c file, since it should never be exposed [04:32] should be typedef'd to that name [04:32] member names should line up [04:33] userdata -> data [04:33] proxy should probably go first too [04:33] Keybuk: for which callback? [04:34] argument to the async dispatch function [04:34] Keybuk: ah, nm, I misunderstood for a moment [04:34] struct nih_blah_blah -> NihBlahBlah [04:34] I wouldn't call it that though [04:34] make it similar to the DBus Name [04:34] NihDBusPendingCall or something [04:34] Keybuk: you omit the CamelCase form on another struct in dbus.h [04:35] do I? [04:35] pretty sure [04:35] I bet I typedef'd it first ;) [04:35] NihDBusInterface? [04:35] line 44: typedef struct nih_dbus_interface NihDBusInterface; [04:36] Keybuk: ah. so it is. [04:36] I can't quite work out what you're doing with the callback definition in asyncDispatchPrototype() - a typdef would definitely help here [04:37] and then it'd be obvious you've forgotten all about in_args ;) [04:37] Keybuk: line 1283 [04:37] oh [04:37] they should go first [04:38] Keybuk: I figured put the consistent-between-all stuff first [04:38] proxy, input arguments, callback, data [04:38] my_str_to_int (proxy, "foo", get_int, NULL); [04:38] reads the right way then [04:39] yeah, I can see that.. [04:40] you don't actually check whether the pending call completed? [04:40] in fact [04:40] dbus_pending_call_steal_reply doesn't return NULL to indicate out of memory [04:41] it returns NULL to indicate no reply received yet [04:41] in case of timeout, dbus_pending_call_steal_reply returns an error [04:42] in fact, how do we deal with errors at all? :) [04:42] oh yeah, I hadn't done that yet had I? [04:42] # FIXME: Reply isn't the best parent for this, but proxy isn't here. [04:42] comment doesn't make sense given you use proxy ;) [04:42] oh, fixed it [04:43] should the proxy be passed to the callback function? [04:43] I think it makes sense for it to be [04:43] in case the callback wants to make more calls [04:43] an error handler is going to look like void (*error_handler) (void *data, NihDBusProxy *proxy; [04:44] which I guess means you have to provide both [04:44] my_str_to_int (proxy, "foo", get_int, deal_with_error, NULL); [04:44] but it's probably ok for the error handler to be NULL [04:44] actually [04:45] no it's not, let's make that required if a callback is givne [04:45] nih_alloc (..., struct) -> nih_new ()! [04:46] you don't provide a free function for the pending call [04:46] that's kinda important, since otherwise you'll leak the async data structures [04:46] the free call might want to do something like call the error handler [04:46] setting some useful error [04:47] ok [04:47] so for str_to_int, that'd look something like [04:47] my_str_to_int (proxy, "1234", int_reply, error_handler, data); [04:47] with the others being [04:48] void int_reply (void *data, NihDBusProxy *proxy, int num); [04:48] void error_handler (void *data, NihDBusProxy *proxy); [04:48] ( the error would be retrieved with nih_error_get() ) [04:55] Keybuk: we could save a function pointer by just adding an "int succeeded" to the callback [04:57] error_handler would be more consistent with other functions [04:57] e.g. nih_io [04:59] Keybuk: did you look at the testcase? [04:59] there's a bit of conventional intrigue there too [04:59] I did, my eyes have not recovered ;) [05:00] Keybuk: I do like gcc's mandate of the auto keyword in that context [05:00] "hey, we need to break a grammar ambiguity. Know any keywords that aren't busy?" [05:06] hah [05:07] Keybuk: if you have a better way to structure those I'm open to it. What I like about this method is that the tests still read in the order they're run. [05:07] I'll either hate it ... [05:07] or convert everything else to use them ;) [05:08] ngggarrrrgh [05:08] huh? [05:08] the valgrind error can be replicated with the absolute simplest of code [05:08] c = job_class_new (NULL, "foo"); [05:09] j = job_new (c, NULL); [05:09] nih_free (c) [05:09] that's ugly [05:09] the strange thing is that this should all just work now [05:10] the whole point of refs is that if you have a pointer, you have a ref [05:10] so they should collapse cleanly *sigh* [05:11] one thing I wish we could do (and I'm nearly certain we can't) is have a NIH_FRAME macro instead of nih_local, and you simply parented things to it to make them have a reference held by the current stack frame [05:11] I considered that approach [05:11] I decided nih_local was nicer [05:12] I like having the frame parent, but the things you'd have to do to make it work would offset any nicety. [05:12] you'd have to hide a var somewhere in the function somehow to run your cleanup [05:13] or pillage the stack manually [05:13] having the frame parent feels a bit like assembler [05:13] or keep a costack and call everything that uses it through wrappers [05:13] just saying "this is local" feels a bit like python ;) [05:13] I think its the word "frame" [05:13] the aim is to simplify the code [05:14] you'd have to do something like: [05:14] { [05:14] NIH_LOCAL local; [05:14] char *str; [05:14] ... [05:14] str = nih_strdup (local, "foo"); [05:14] } [05:14] it kinda gets lost in the fog a bit [05:14] the need to declare it is what kills it [05:14] and that's what I can't factor out [05:15] yeah [05:15] if that were [05:15] { [05:15] char *str; [05:15] ... [05:15] str = nih_strdup (NIH_LOCAL, "foo"); [05:15] that would be ok [05:15] but I don't think you can declare variables right there ;) [05:15] and even if you could, the second use of NIH_LOCAL would break it [05:15] what I /really/ wish is that you could do this: [05:15] (and I see what causes this valgrind bug now ... [05:16] it's the ancient thing-is-in-a-hash-table bug) [05:16] void foo (char nih_local *bar) {... [05:16] class->jobs is a hash table [05:16] job is in that [05:16] and it would make bar nih_local and /ref/ it [05:16] when you free job, it removes itself from the hash [05:16] but if you free class, the hash should be freed first [05:16] sadmac: huh? [05:17] how does that work [05:17] it's an arg passed in, it's inherently already ref'd by something and you don't need to care [05:17] Keybuk: pthread_create() [05:17] * Keybuk wonders why this valgrind bug doesn't affect old nih_alloc [05:18] timestamp: Thu 2006-12-21 00:24:05 +0000 [05:18] message: [05:18] * nih/io.c (nih_io_message_new): Do not assign a default list [05:18] destructor, it's not in a hidden list and there are bad consequences [05:18] of freeing a child with a default destructor if the list its in gets [05:18] freed first; new rule - default destructors only if the list is hidden [05:18] and never freed! [05:18] ... [05:18] especially since I apparently hit it over 2 years ago :p [05:19] Keybuk: if we go thread-safe, stack references are more important, because a piece of code referencing a function doesn't tell us as much about other pieces of code that might be dealing with the memory area [05:19] that's when things like this become interesting [05:19] but back to your valgrind issue... have a fix? [05:26] no, not yet [05:26] I'm trying to understand why it's breaking now first [05:28] Keybuk: while we're here, I had a hypothesis: why do you put the function types on the line above the name when defining and on the same line when declaring? [05:29] common C practice [05:29] grep ^function_your_looking_for *.c [05:29] as a way of locating the code [05:30] part A confirmed [05:30] Keybuk: part B: do you use cscope? [05:31] nope [05:31] mhm [05:31] why? [05:32] Keybuk: the theory is that if you used cscope you wouldn't put the function definition on two lines' [05:32] Keybuk: because you'd be able to find the function no matter how it was formatted [05:33] I use ctags, which are pretty much the same thing [05:33] but it's something I find useful in other people's code [05:33] so I do it too [05:33] and it has the advantage of making a function definition really obvious [05:35] gives you more space for arguments too [05:35] * sadmac used to pride himself on the distant right margin of his VB code [05:35] ah was I ever that young? [05:37] heh [05:37] my C coding style is kinda wacky really [05:37] like the way I line up argument and variable names [05:38] and if (! foo) [05:39] Keybuk: the putting spaces between function names and open parens kills me. especially now that I'm paid to be in kernel code [05:39] haha [05:39] someone once said that there are only two types of programmers [05:40] those that put a space between function names and their parameters, and those that don;t [05:40] Keybuk: and the guys who wrote procmail [05:40] that's definitely a new species [05:41] fd.o's C style guidelines are pretty backward too [05:41] 2-space tabs? doing it wrong. [05:41] here's a question for you then [05:42] if you don't like foo(...) [05:42] do you do for(...) ? [05:42] Keybuk: no, because for is yellow in vim [05:42] yellow things get a space before the args [05:43] except for sizeof [05:43] because linus says so [05:45] sizeof shouldn't have ()s [05:45] sizeof int [05:46] you take the size of nicer things than I do [05:46] sizeof(typeof(**x)) [05:46] typeof doesn't have ()s either [05:47] besideswhich [05:47] sizeof/typeof is meaningless [05:47] sizeof **x [05:47] is the same [05:47] ...whoa... [05:47] you're right [05:49] Keybuk: what about those people that name the function arguments outside of the declaration header? [05:50] those people need to stop denying the 80s happened [05:50] or go code COBOL in a hole somewhere [05:51] HOLBOL? [05:51] arguments outside the header is old-style K&R C [05:51] we program in ANSI C nowadays [05:51] sort of [05:55] Keybuk: can we add cscope.out to the ignorefile? [05:57] no ;) [05:57] for the same reason I don't put TAGS in there [05:58] (you should only ignore output expected to be generated by the ordinary build process) [05:59] Keybuk: does bzr have a local ignore? [06:01] I don't think so [06:01] damn [06:01] * sadmac misses git [06:02] oh, yes [06:02] ~/.bazaar/ignore [06:04] cool [06:04] bug! [06:05] as soon as I added a line to ~/.bazaar/ignore, all the object files showed up in bzr status [06:05] oh, I think that's deliberate [06:05] bzr has a built-in global ignore [06:05] if you make a new one, it overrides bzr [06:06] can I source the normal one? [06:06] hmm [06:06] did you not have a .bazaar/ignore already ? [06:06] you just did echo > .bazaar/ignore [06:06] didn't you? [06:06] FUUUUUUUUUUUUUUUUUUUUUUUUUUUU [06:06] rm it [06:07] run bzr ignored [06:07] then edit it ;) [06:07] superior usability! [06:08] you can't use that word [06:08] you like git [06:09] I /can/ however use my vcs [06:09] * sadmac uses interactive rebase to do obscure things you can't even imagine in O(1) [06:09] I can use git too, I just strongly dislike it [06:09] Linus tells you not to rebase [06:09] he says every time someone rebases, he kills a kitten [06:09] interactive rebase is better than rebase [06:10] it basically lets you put everything in the repo in any order you want. [06:10] its like using ed on the object files, but you can play louder techno music [06:11] you're not supposed to fuck with the repo [06:11] I don't get much other action, and the repo understands my needs [06:11] don't judge me, what we do is beautiful [06:13] :D [06:13] oh /repo/. I thought you said horses. [06:13] how equus of you [06:14] I don't think he actually fucked the horse. [06:14] He stabbed the horse. He rode the horse naked. [06:14] Not sure you could stage a fucking in the way that play was supposed to be staged [06:18] no idea [06:18] its kind of a funny image actually... [06:18] http://www.asiaarts.ucla.edu/media/images/EQUUS%201.jpg [06:19] that's pretty funny already. If that broke down into some hardcore I'd be seriously amused. [06:20] heh [06:20] * Keybuk spots the basic difference between old nih_alloc and new nih_alloc [06:21] o yeah? [06:21] revno: 455 [06:21] committer: Scott James Remnant [06:21] branch nick: libnih [06:21] timestamp: Mon 2007-10-15 23:14:14 +0100 [06:21] message: [06:21] * nih/alloc.c (nih_alloc_using, nih_alloc_reparent, nih_realloc): [06:21] Change the order in which children allocations are stored in the [06:21] list such that the last allocation is freed first rather than [06:21] the other way around. This solves issues of children being stored [06:21] inside an allocated hash table which will be freed first. [06:23] in other words, old nih_alloc frees things in the opposite order to that they are allocated in [06:23] that working implies that the parenting ordering is a subset of the allocation ordering [06:24] its perfectly possible to: [06:24] 1) allocate item with no parent [06:24] 2) allocate hash table [06:24] 3) stick item in hash table [06:24] in which case this would break [06:24] yeah I never really fixed the original bug [06:24] I just kept moving it around every time I noticed it ;) [06:25] what you should do is not free the memory right away but put it into some kind of list, and then free everything in the list only when its know that all the references are stable. [06:27] nih_free (void *item) [06:27] { [06:28] static int rec_depth = 0; [06:28] ... [06:28] rec_depth++; [06:28] ... [06:28] nih_insert_item_into_free_list(item); [06:29] if (! --rec_depth) { [06:29] nih_free_all_free_list(); [06:29] yeah, that's the plan [06:29] finalise the object first [06:37] Keybuk: where do I move the NihAsyncNotifyData struct so it shows up in the C files? [06:38] you might have to define something to do that [06:39] mmh [06:39] * sadmac thinks about rewriting all this with a templating engine again [06:41] ...with child in older sibling list [06:41] BAD: block 0xacd108 (child->entry.next) freed unexpectedly [06:41] at tests/test_alloc.c:243 (child_destructor_test). [06:41] sweet [06:41] have a testcase then? [06:42] I think so [06:42] though one passes but fails valgrind [06:42] oh, hah [06:42] because I'm an idiot [06:42] my test case uses TEST_FREE_TAG ... which uses nih_alloc [06:42] can't test nih_alloc with TEST_FREE_TAG you dolt [06:56] right [06:57] I now have two test cases [06:57] that both work [06:57] if you use _add () the first test case fails [06:57] if I change it to _add_after () the second test case fails [07:01] Keybuk: ok, last I saw I asked about typedefs [07:06] I didn't see that [07:06] Keybuk: is there a place to put the typedefs for the callbacks [07:07] anything above that? [07:07] 01:43 * sadmac fixes a longstanding problem with his vimrc [07:07] then before that you said you were an idiot [07:08] heh [07:08] not sure if there's a place in the python code [07:08] I certainly meant to have one, but may not have added that func [07:09] should go at the top, before externPrototypes [07:38] I'm sleeping now. [07:47] heh [07:47] I'm just about to do that [07:47] I'm worried [07:47] my code worked first time [07:54] Upstart's test suite largely works too [07:54] there's a few failures, but I don't _think_ they are related [08:07] (and frankly, the d-bus stuff works - and that really abuses nih_alloc :p) [08:08] http://bazaar.launchpad.net/%7Escott/libnih/trunk/revision/639 [08:09] in summary: first call all the destructors, then free them all [08:09] but rather more efficient without the need to recurse ;) [21:23] right [21:23] you and me, valgrind... [21:25] Keybuk: what about you, him, and valgrind? [21:26] going to fix these remaining test case failures [21:26] I'm also trying to decide whether it's a bug to call nih_free () on an object with a parent [21:26] ion_: what do you think? [21:27] I’d lean on only allowing to free by unrefing. [21:27] uh, towards [21:27] what if you never allocated it with a reference in the first place? :) [21:28] Keybuk: I'd lean toward /that/ being a bug [21:28] or rather impossible [21:28] well, you have to start somewhere? [21:28] Ah, right, parentless objects don’t have a reference. [21:28] Hmm [21:28] otherwise you'd need a global reference, which is basically the same as NULL ;) [21:28] Keybuk: exactly [21:28] so why not use NULL? [21:29] the disadvantage of a global reference, and the reason nih_alloc doesn't have one, is that valgrind thinks the object is in use ;) [21:29] Keybuk: yes. Use NULL by all means. [21:29] so doesn't bitch if you forget to free [21:29] that was the other main reason I opted for "nih_local" and NULL rather than NIH_LOCAL as the parent [21:30] Keybuk: you could have a debug mode where you htonl() the refs that are from the root object [21:32] huh? [21:32] Keybuk: "encrypt" references from the root object when -DDEBUG so valgrind can't read them [21:33] How about just making unref() free parentless objects, and behave as before with objects with parents? [21:35] that involves having a debug mode though ;) [21:35] ion_: nih_unref (ptr, NULL) type thing? [21:38] Or maybe make nih_free barf if the object has parents? [21:40] or make nih_free only do something if the object has parents? [21:46] How should it be used then? [21:46] If you nih_new() without a parent and nih_free() it, nothing would happen, right? [21:47] I mean if the object hasn't parents [21:47] so nih_free() matches nih_new(NULL, ...) [21:47] Keybuk: what's the difference between what you get from nih_alloc(NULL, ...) and what you have in nih_unref() when you've removed the last ref and its about to be freed [21:47] and nih_unref() matches nih_ref() or nih_new(ptr, ...) [21:47] sadmac: none [21:47] if you like, nih_alloc (NULL, ...) gives you something that's unreferenced, but not free [21:47] a bit like gobject's sunken ref ;) [21:47] Keybuk: that's my issue. its identical to the dead state [21:48] it means you can allocate something, add a reference, and then not worry about it [21:48] you don't have to drop your own [21:49] Keybuk: IMHO such an object persisting for any amount of time is as bad as a pointer to already freed memory persisting for any amount of time. [21:51] it varies [21:51] some reference-counting systems are strict [21:51] when you allocate, what you have returned has a single reference [21:52] if you pass that to anything and it takes a reference, it must add *another* reference [21:52] and if you don't want it anymore, you *must* drop the single reference you had when it was allocated [21:52] D-Bus's is like that [21:52] Keybuk: why does nih_unref() free the object then? [21:52] the problem with these is that it's easy to forget to drop the reference you got when you allocated it, or easy to forget to take one when you want it [21:52] Keybuk: its a valid object still [21:52] so others have the concept of a floating reference [21:53] when you allocate, what you have has only a floating reference [21:53] if anything references it, the floating reference is sunk and turned into a real reference [21:53] so when you pass it to things that take a reference, it gets turned into a real one [21:53] and since that happens, you don't need to worry about unrefing when you don't want it anymore [21:53] ... [21:53] non-deliberately, nih_alloc turns out to be a bit like the last one [21:54] you can allocate with a reference, or without [21:54] if you allocate without, it kinda behaves like a non-explicit floating reference [21:54] in reality, it's an unreferenced objetc [21:54] this works nicely for things like nih_local [21:54] since that only discards unreferenced objects [21:55] indeed, there's a call for - nih_discard() - which is a safety "I'm done with an unreferenced object, but am not sure that anyone else took a reference" call [21:55] and you can return an object to unrefernced as well - nih_unref_only () [21:56] this is all a bit ... strange really ;) [21:56] it's what happens when you design an API by evolution [21:56] Keybuk: I can see the argument for it existing, but I don't like it as a long-term condition [21:56] especially when you turn a simple halloc rip-off, into a talloc rip-off into something that afaik is unique [21:56] Keybuk: I'd like to think of it as mid-transaction database inconsistency [21:57] Keybuk: its an error for such a thing to exist, but we defer the error on the grounds of "it'll not be that way anymore in a few lines" [21:57] Perhaps rename nih_free as nih_discard (deleting the present nih_discard) to avoid confusion and make it free the object if it has no parents. [21:57] ion_: err, that would be exactly what nih_discard does right now, no? :p [21:57] Indeed. [21:58] though would it error if it has parents? [21:58] Keybuk: so rather rename nih_discard to nih_free, and kill the current nih_free [21:58] (nih_discard doesn't) [21:58] No, ignore it if it has parents. [21:58] It shouldn’t be called nih_free if it might not free the object IMO. [21:59] nih_free always frees right now [21:59] even if it has parents [21:59] I tend to agree there though, nih_free would be a bit odd if it didn't always free [21:59] Yes, i suggest getting rid of it. :-) [21:59] Keybuk: nih_free not always freeing is weird [22:00] Keybuk: but if you have an interest in API consistency (and it /is/ kind of convenient that we don't have to rewrite all that much right now) it might be better to keep the old symbol/ [22:00] So the norm would be to nih_discard anything you allocated without a parent, and nih_unref anything you referenced. No nih_free. [22:00] heh [22:01] I just made nih_free assert [22:01] (if there were parents left) [22:01] lots of things broke [22:01] :-) [22:01] mostly test cases, by the looks of it [22:01] Keybuk: what I would prefer is that if you are going to allocate without a parent, you /must/ assign the pointer to an nih_local. dunno what C gives us to enforce that though. [22:03] dunno, dig through the gcc ref manual ;) [22:03] I've learned to like that thing [22:04] Keybuk: I wonder what happens if you do void * nih_local my_func () {... [22:05] the attribute will apply to the function instead of the return value [22:05] Keybuk: but it can't because its not a function attribute [22:06] so gcc will error [22:06] return types don't have variable attributes ;) [22:06] probably [22:06] the __attribute__ binds right