* Marc Slemko wrote: > But yes, there are some issues they point out that should be fixed if they > are still present. Fixing them certainly won't significantly change the > overall software quality though.
Well, let's have a look. My first analysis: ID 1 seems to be invalid, the preconditions code path cannot happen. (second and last precondition are in opposite of each other, because current_provider == conf->providers in this case) ID 2 ... hmm, am I blind or don't we have an abort_fn registered in our pools? Why? That would solve a lot of problems, IMHO. If there's an abort_fn registered, all IDs referencing to this one are invalid. ID 3 is valid. We should bail out and return -1 if tag == NULL, I think. ID 4: Not sure. What shall happen, if a bucket cannot be created? ID 5: see ID 2 ID 6, 7, 8: see ID 4 ID 9: seems to be invalid. strchr(segstart, '\0') cannot return NULL on an already valid C-string. ID 10: see ID 2 ID 11: see ID 4 ID 12: hmm, I don't seem to understand the critical loop... Could someone explain it (why do we start with i=1?). An if(args) wrapper should solve it anyway. ID 13: we should leave it as is, because it shows us errors in the caller functions. (would call it "by design"). ID 14: see ID 13 ID 15, 16, 17, 18: see ID 4 ID 19: ap_strchr_c(valid_string, '\0') cannot be NULL. see ID 9 ID 20: seems to be invalid? ID 21: don't know, I'm not an APR guru ;-) ID 22, 23: hmm. by design, IMHO, see ID 2 ID 24: see ID 4 ID 25: very valid! ID 26: seems to be valid, needs an if (pTail) wrapper. ID 27: see ID 22 ID 28: haha! Already fixed two days ago or so. ID 29: should be fixed. ID 30: errr, cannot happen, but we should bail out (assert?) ID 31: correct, though I don't think we should double check n. More opinions and reviews would be helpful, of course. nd
