**** BEGIN LOGGING AT Wed Apr 21 02:59:56 2010
Apr 21 03:18:19 <zhenhua> denkenz: ping
Apr 21 03:19:16 <zhenhua> denkenz: i am trying to refactor the emulator.c. how could multiple emualtors co-exist in the system? in core or as plugins.
Apr 21 03:24:35 <denkenz> in core, just make them into another atoms
Apr 21 03:24:42 <denkenz> we can have multiple atoms of the same type
Apr 21 03:27:23 <zhenhua> so i guess we will have src/atemulator.c, src/hfpemulator.c, etc. right?
Apr 21 03:28:25 <zhenhua> not quite sure about that
Apr 21 03:29:08 <zhenhua> the real handling work, such as ata or atd will be handled by src/voicecall.c directly, right?
Apr 21 03:29:24 <denkenz> we probably won't have anything like src/atemulator
Apr 21 03:29:33 <denkenz> simply handle everything in the individual atoms
Apr 21 03:30:30 <zhenhua> so in struct emulator, we have multiple atoms, one for at, one for hfp, something like that.
Apr 21 03:30:47 <zhenhua> s/emulator/ofono_emulator
Apr 21 03:31:07 <zhenhua> not sure whether it works.
Apr 21 03:31:13 <denkenz> yeah, maybe something like ofono_emulator_get_type() -> DUN / HFP
Apr 21 03:31:25 <denkenz> and OFONO_ATOM_EMULATOR
Apr 21 03:31:34 <zhenhua> okay
Apr 21 03:31:39 <denkenz> or even OFONO_ATOM_EMULATOR_HFP, OFONO_ATOM_EMULATOR_DUN, etc
Apr 21 03:32:02 <denkenz> We can decide what looks better later
Apr 21 03:32:06 <zhenhua> sure
Apr 21 03:32:50 <zhenhua> again, my question is:  the real handling work, such as ata or atd will be handled by src/voicecall.c directly, right
Apr 21 03:37:52 <denkenz> yep
Apr 21 08:01:00 <akiniemi> denkenz: I pushed a patch that should fix the compile problems you had yesterday
Apr 21 17:10:51 <holtmann> akiniemi: What is this: return (GIsiModem *)(void *)(uintptr_t)index;
Apr 21 17:12:45 <holtmann> What are you actually trying to fix here. I don't want nasty casting like this inside the codebase.
Apr 21 18:13:52 <matgnt> denkenz: thanks for your help with the wavecom modem and getting it to the master branch :)
Apr 21 18:14:21 <denkenz> matgnt: No worries ;)
Apr 21 18:55:28 <akiniemi> holtmann: index is an unsigned, and on 64bit, that uintptr_t cast gets rid of a warning
Apr 21 18:58:25 <holtmann> Three casts in one statement. That is just wrong.
Apr 21 18:58:31 <holtmann> Can we fix this properly.
Apr 21 18:59:45 <denkenz> btw, in theory this should work:
Apr 21 19:00:05 <denkenz> Since linux/if.h includes net/if.h on some systems and conflicts with others
Apr 21 19:00:18 <denkenz> You can simply include only linux/if.h and check for the net/if.h define
Apr 21 19:00:37 <denkenz> If defined, don't do anything, else extern declare the functions that you need
Apr 21 19:02:14 <akiniemi> denkenz: I just moved all uses of if_nametoindex in a separate file
Apr 21 19:03:00 <akiniemi> In fact that made more sense anyway, since the function had a different prefix anyway...
Apr 21 19:03:35 <akiniemi> holtmann: hmm. Maybe that uintptr_t cast is all that is needed, actually.
Apr 21 19:04:21 <denkenz> Imo that screams kludge
Apr 21 19:04:54 <akiniemi> denkenz: which one? Or both? :)
Apr 21 19:05:07 <denkenz> The dedicated file for a function part
Apr 21 19:05:33 <akiniemi> denkenz: it was already defined in a separate header, modem.h
Apr 21 19:06:23 <akiniemi> I just moved the implementation into modem.c, instead of keeping it in netlink.c. Cleaner, IMHO.
Apr 21 19:07:01 <akiniemi> courmisch, around? I think those casts are your handywork. ;)
Apr 21 19:07:10 <denkenz> I still don't get why casts are even necessary
Apr 21 19:07:18 <denkenz> All of this just screams crapiness
Apr 21 19:07:37 <courmisch> err no
Apr 21 19:07:39 <denkenz> Maybe they are, but there are no comments to explain it
Apr 21 19:07:48 <courmisch> gcc complains if you cast an int of non pointer-size to pointer
Apr 21 19:07:59 <courmisch> and it complains if you cast an integer to a pointer
Apr 21 19:08:05 <courmisch> to so you need to casts in a row
Apr 21 19:08:15 <courmisch> s/to c/two c/
Apr 21 19:08:25 <courmisch> for integer -> pointer anyway
Apr 21 19:08:35 <courmisch> the other direction only needs one cast to (u)intptr_t
Apr 21 19:08:38 <denkenz> Then use G_INT_TO_POINTER
Apr 21 19:08:43 <denkenz> or G_POINTER_TO_INT
Apr 21 19:08:48 <denkenz> Where the magic is done for you
Apr 21 19:09:08 <courmisch> I am not a fan of the glib type system to say the least
Apr 21 19:09:48 <denkenz> Point is, those macros don't double cast on 32 bit systems
Apr 21 19:10:11 <courmisch> so what?
Apr 21 19:11:02 <denkenz> The fact that it is immediately clear wtf is going on and I don't have to suffer broken builds
Apr 21 19:11:02 <courmisch> casting does not cost anything at run-time with any sane compiler
Apr 21 19:11:35 <courmisch> using C99 is way clearer to me than some proprietary glib stuff
Apr 21 19:11:53 <courmisch> but hey, I don't care anymore.
Apr 21 19:12:01 <courmisch> I am just totally not going to "fix" it
Apr 21 19:12:17 <denkenz> Yeah, that can be seen from the code quality
Apr 21 19:12:39 <courmisch> that's an insult you know
Apr 21 19:14:01 <holtmann> courmisch: Don't cast an int to a pointer in the first place.
Apr 21 19:14:11 <denkenz> not really, we are quite clear about coding style and conventions
Apr 21 19:14:25 <denkenz> If you guys cannot follow them, then I can 'insult' the code all I want
Apr 21 19:14:37 <courmisch> holtmann: putting a sole int in a malloc is retarded
Apr 21 19:14:57 <courmisch> and exposing an int as an "opaque" type is just as retard
Apr 21 19:15:08 <holtmann> The casting mess is a disaster waiting to happen.
Apr 21 19:15:37 <courmisch> please don't blame me for lack of type safety in C
Apr 21 19:15:55 <courmisch> you may want to use C++ if it's a problem (not that I'd recommend it)
Apr 21 19:16:33 <courmisch> and -Werror and no #ifndef is worse disaster to me
Apr 21 19:16:42 <holtmann> That is just crap.
Apr 21 19:16:44 <courmisch> but we can only agree to disagree I'm afraid
Apr 21 19:17:03 <courmisch> the simple fact is that different compiler/version yield different warnigns
Apr 21 19:17:15 <courmisch> sometimes plain wrong, sometimes contradictory across compiler/versions
Apr 21 19:17:25 <courmisch> hence -Werror by default is insane
Apr 21 19:17:35 <courmisch> I have seen it in other projects
Apr 21 19:18:05 <holtmann> Funny that we haven't had any major problems with it inside BlueZ.
Apr 21 19:18:24 <courmisch> I wouldn't know about bluez
Apr 21 19:18:32 <courmisch> but seen it in both VLC and, hmm oFono
Apr 21 19:19:04 <denkenz> So the golden rule of all software engineering projects: If there's a coding standard, fucking follow it
Apr 21 19:19:08 <courmisch> the "cast increases needed alignment" is just unfixable on ARM
Apr 21 19:19:14 <denkenz> Nobody gives a shit if you agree or disagree with it
Apr 21 19:19:39 <denkenz> If there's a problem, raise it on the list
Apr 21 19:19:58 <courmisch> I raised them a long time ago
Apr 21 19:20:03 <courmisch> to be ignored like shit
Apr 21 19:20:10 <courmisch> so stop the BS please
Apr 21 19:21:04 <denkenz> So raise it again, that is how it works
Apr 21 19:22:38 <courmisch> I have more pressing issues that harass people who've obviously decided to ignore or disagree anyway
Apr 21 19:24:20 <holtmann> So either you guys use GArray for this. Or you allocate a proper GIsiModem struct with the idx as sole element.
Apr 21 19:24:42 <holtmann> The casting is just insane.
Apr 21 19:25:16 <holtmann> If you wanna save the few bytes of allocated memory, then GArray it is.
Apr 21 19:26:36 <holtmann> Or just open code your index list by yourself, but stop blaming gcc or -Werror for this.
Apr 21 19:30:35 <denkenz> Ok, my kingdom for a comment
Apr 21 19:30:42 <denkenz> wtf is is _GIsiModem anyway?
Apr 21 19:32:55 <denkenz> If this is just an int, then you guys are off your rocker, seriously
Apr 21 19:33:57 <holtmann> If it is an int, then just lets call this as it is, an int.
Apr 21 19:34:18 <holtmann> We also don't wrap file descriptors as FileFD *.
Apr 21 19:38:52 <akiniemi> guys, wtf?
Apr 21 19:39:30 <akiniemi> You guys need to cool it. No need to get all worked up about this.
Apr 21 19:39:46 <akiniemi> denkenz: this didn't break your build, mind you.
Apr 21 19:40:35 <denkenz> This is just a symptom
Apr 21 19:40:46 <denkenz> And it has broken the build before
Apr 21 19:41:14 <akiniemi> Well, your hotswap patches broke the isimodem driver, too.
Apr 21 19:42:26 <akiniemi> And I'm sure all will be peachy if you bash us long enough here. Geez.
Apr 21 19:43:34 <denkenz> Simply admitting something needs work and accepting suggestions works too
Apr 21 19:44:08 <denkenz> This casting non-sense needs to go away or be explained to me
Apr 21 19:46:31 <akiniemi> denkenz: so what exactly did you suggest?
Apr 21 19:46:44 <akiniemi> denkenz: apart from suggesting it screams of crapiness?
Apr 21 19:47:20 <denkenz> akiniemi: So my understanding is that GIsiModem is a simple pointer, it has no members
Apr 21 19:47:36 <denkenz> And you guys do all sorts of casting nastiness to make it look like an actual struct
Apr 21 19:47:39 <denkenz> Question is why?
Apr 21 19:49:01 <akiniemi> denkenz: it's an opaque type
Apr 21 19:49:20 <denkenz> where is this type declared?
Apr 21 19:50:22 <denkenz> All I see is: typedef struct _GIsiModem GIsiModem; in modem.h
Apr 21 19:50:29 <denkenz> No definition of struct GIsiModem anywhere
Apr 21 19:51:21 <akiniemi> denkenz: so you
Apr 21 19:51:42 <akiniemi> ...'d be happy with a typedef to unisgned instead?
Apr 21 19:51:54 <akiniemi> or just plain int
Apr 21 19:52:06 <denkenz> I'd be happy if we simply use an if index or an if name
Apr 21 19:52:07 <holtmann> I am fine with just plain int. Like with file descriptors.
Apr 21 19:52:27 <akiniemi> Yeah, that works for me.
Apr 21 19:52:34 <holtmann> And then please use GArray for the list of indexes. The GLib documentation has even an example for that.
Apr 21 19:52:49 <holtmann> Using GList to store an int is kinda messy.
Apr 21 19:53:29 <akiniemi> holtmann: that is probably a side-effect of thinking it's a struct...
Apr 21 19:53:36 <holtmann> akiniemi: My general rule of thumb is that if we need heavy casting like this, then something is way too complex.
Apr 21 19:53:45 <holtmann> And then we need to review the complexity of the code.
Apr 21 19:53:52 <holtmann> That is my main complaint here.
Apr 21 19:54:15 <akiniemi> sure.
Apr 21 19:54:21 <holtmann> akiniemi: And that is the reason why I am enforcing -Werror.
Apr 21 19:55:09 <holtmann> I wanna catch these situation. Otherwise code rots and we get into massive problems with other archs than x86 32-bit.
Apr 21 19:55:11 <akiniemi> I have to say that my last irssi screenful is constructive, but the previous wasn't.
Apr 21 19:56:54 <holtmann> I wasn't following the whole discussion. I just saw your commit with the triple cast and ask for an explanation, then I got distracted with other work.
Apr 21 19:57:18 <holtmann> As I said, my alarm bells go on when I see casting.
Apr 21 19:58:16 <holtmann> We demand as less casting as possible.
Apr 21 19:58:30 <denkenz> And now we can happily get rid of modem.c kludginess
Apr 21 20:00:26 <holtmann> akiniemi: Just blaming gcc or -Werror doesn't fly with me. Difference between 32-bit and 64-bit are known and we all know that gcc is not perfect. However simpler code makes it easy for gcc.
Apr 21 20:02:58 <denkenz> akiniemi: And in the future, if there's something that tricky going on, reject it or make sure there's a huge big fat comment telling the reader wtf is going on
Apr 21 20:03:36 <denkenz> akiniemi: When I see a typedef like that I expect an actual struct somewhere, and my alarm bells go off when its being cast to an int
Apr 21 20:36:44 <balrog-k1n> the first cast, to GIsiModem *, makes no difference for gcc
Apr 21 20:39:13 <denkenz> akiniemi: And while I'm at it, can isi drivers be tweaked to accept a g_isi_client structure instead of GIsiModem?
Apr 21 20:39:38 <denkenz> akiniemi: Then your drivers become identical in structure to AT drivers and we don't have to create a dedicated socket for each and every atom
Apr 21 20:41:30 <akiniemi> denkenz: what's the problem in creating a socket per atom?
Apr 21 20:41:48 <denkenz> akiniemi: No problem, other than don't if you don't need to
Apr 21 20:42:12 <denkenz> akiniemi: Less is more here
Apr 21 20:42:21 <akiniemi> denkenz: we thought about this, but then the transaction ID space would be shared across all atoms
Apr 21 20:42:34 <holtmann> Why is that a problem?
Apr 21 20:43:55 <akiniemi> denkenz: well, that's only 8 bits, but more importantly, the modem services don't always behave well
Apr 21 20:44:06 <akiniemi> s/denkenz/holtmann
Apr 21 20:44:32 <denkenz> akiniemi: Ok I give you a reason
Apr 21 20:44:38 <denkenz> akiniemi: GIsiClient structure is *huge*
Apr 21 20:44:51 <holtmann> Meaning that if you intermix transaction ID with different modem services you have problems.
Apr 21 20:45:23 <holtmann> Sounds like your modem firmware guys have to actually fix their modem firmware.
Apr 21 20:45:38 <denkenz> Even creating 1 client / service is better
Apr 21 20:45:51 <denkenz> e.g. call barring, ussd, call settings share 1 Client
Apr 21 20:45:58 <denkenz> Instead of each one creating its own
Apr 21 20:46:00 <akiniemi> denkenz: and gprs, gprs context
Apr 21 20:46:07 <denkenz> correct
Apr 21 20:46:21 <akiniemi> denkenz: that's on my todo list
Apr 21 20:46:51 <akiniemi> holtmann: where to start... ;)
Apr 21 20:47:12 <denkenz> Your casting problems and GisiModem struct all go away if you do it that way
Apr 21 20:47:39 <akiniemi> denkenz: that's a lot more work than just making GIsiModem a uint
Apr 21 20:47:45 <denkenz> Everyone is less stressed, nobody is calling code kludgy, etc ;)
Apr 21 20:48:33 <denkenz> Good things take time, as long as I know it is being fixed I'm cool
Apr 21 20:55:51 <denkenz> akiniemi: And don't get me started on the contents of GIsiClient, can someone please explain it to me why we're not using linked lists?
**** ENDING LOGGING AT Thu Apr 22 02:59:57 2010