Re: [libvirt] [PATCH] Initial implementation of new job control api

On Wed, Jun 18, 2014 at 05:59:47PM -0400, Tucker DiNapoli wrote: You should use VIR_JOB_LAST for easy extension and proper size checking. I've just now discovered that running make syntax-check would tell you the same. These are probably general enough, yet. We prefer having these in a separate functions. Not only can you trace them better when debugging (for such purposes I use CFLAGS="-ggdb -O0"), but it also shouldn't add too much of an overhead with proper compiler optimizations. This creates code with two flaws. One is readability, because it can return from function (skipping possible clean-ups, e.g. in future code) even though it's not obvious from the name. Second one is that there is no need to have a semicolon after the macro (which confuses some editors). The usual workaround is: #define ASDF() \ do { \ asdf(); \ while (0) While the resulting code looks quite nice with all these, I'm not convinced we want all of them. Is _ATOMIC even necessary? You're locking the structure anyway in some cases. Not using the atom ops might be better, as Daniel suggested: https://www.redhat.com/archives/libvir-list/2014-June/msg00194.html If there's going to be a lot of code not related to the job hashtable, it might be worth separating that into separate file (clearly stating that it's simplified and thus not a hash table per se). Why not running the initialization once per binary that uses it? I don't expect it will be needed anywhere else than the daemon, or am I wrong? Maybe we can mak it part of virInitialize() or similar. You're running the virOnce() anyway in lookup() and hashadd(). You're not checking the value of init_err anywhere. How's this id going to change when the daemon is restarted? It now just occurred to me that we haven't even discussed how the jobs will be handled on daemon quit/restart. How about just keeping a sparse window in which there would be jobs ordered by their internal ID? You would avoid those linked lists and have a O(log(n)) lookup, which is not that bad with few jobs. And we have _APPEND _INSERT and other macros to handle array elements. If you really want O(1), I think using the hash table we already have is not such overkill as redefining another hash table in different part of the code. And the functions you need to put in virHashCreateFull() are trivial. We require indenting nested defines, one space per level, in this case like this: #ifndef VIR_JOB_REHASH_THRESHOLD # define VIR_JOB_REHASH_THRESHOLD 0.8 #endif If you have cppi installed, make syntax-check would have told you that, that applies to both whole files. The same as for job locking applies here. BTW, why are you casting the pointers? Also having it in a function checks for pointer- and type-correctness. [...] There is no way this function returns -1, but that's a nitpicking (from my side) with something that might change. However, having one error value is fine unless you really need to fine-tune the behaviour in the caller. That brings me to another thing I wanted to mention. You're not reporting errors anywhere in these files. Do you expect all the callers sharing similar (or most probably the same) error reporting? If you do it here, it will be much nicer and smaller code in those callers. What if the job is locked and being worked with? It should be either locked or you should use reference tracking (counting) which we have in virObjectLockable, that you can make a parent of the job (see how it's used in the code). Also there should be strict rules for the order of locking these elements, so most of the deadlocks are avoided (see for example src/qemu/THREADS.txt where domain and driver locking is explained). This is one of the reasons why you should probably start with the libvirt hash table, concentrate on the job control and leave possible further optimizations for later. This way we'll be dealing with tuning the hash table implementation that is not really part of the project. [...] why is this 0x4? Did you mean to use it as a flag? I don't think there is a job type that could be more than one of these. Not specifying the values will make them consecutive, and when there are no holes, VIR_ENUM_IMPL will work as expected (that's probably why you had "4" in there, right?). Some of these flags are not really flags and might be separated into 'status' for example. I guess the comment here is here because most of the changes are atomic, am I right? If it's not necessary now, leave it out and add it when necessary, that way we won't leave in case it won't be needed. You can do that by defining the struct in the .c file only, that way any other file will have to use accessors and it will be checked by the compiler automagically. This fits better the commit message or underline note in the commit message. Shouldn't these be in the privateData then? Or will these be needed for every job? I'm hesitant to have x{Total,Processed,Remaining} for each x and adding new ones even when only some driver will need them, however, this is internal and can be done lately. It's also pre-existing, so redesigning it might wait (a lot). s/corrsponding/corresponding/ However, the rest is documentation and I only skimmed it. Few pointers though, spaces between the functions will make it more readable, if you start the sentence about return values with "Return(s)", our documentation parser will understand it and use it as such, but that applies to public API only. Martin