Code review comment for lp:~compiz-team/compiz/compiz.fix_1130679

Revision history for this message
Brandon Schaefer (brandontschaefer) wrote :

Looks good, and all the tests are passing. Just a few need fixings or information:

442 + CCSDynamicBackend *dynamicBackend = calloc (1, sizeof (CCSDynamicBackend));

451 + dbPrivate = calloc (1, sizeof (CCSDynamicBackendPrivate));
Though it should be implicit (a cast from a void* -> CCSDynamicBackendPrivate), I would guess if you bump of the compiler flags you would get a warning. Do you want add in the cast? (CCSDynamicBackendPrivate*)call....

1767 + CCSString *string = calloc (1, sizeof (CCSString));
2122 + char *importProfilePath = calloc (1, sizeof (char) * importProfilePathLength);
Same as above...though I don't think it'll be an issue (ignore if you would like!)

956 + char *curVal;
965 + free (curVal);
Looking at what ccsIniConfigFileReadConfigOption does, it can return ccsReadGlobalConfig(option, value) then if it can't find the config file it returns FALSE. This leaves value uninitalized. Though I don't think it'll happen it would be safer if we inited curVal = NULL, and before the free check its not NULL.

1661 char *val;
Same concern as above.

1737 + int len = strlen (item->d_name);
1738 + const char *lastEight = item->d_name + (len - 8);
1739 + const char *lastFour = item->d_name + (len - 4);

Not knowing the context, do we know that iem->d_name is guaranteed to have a char* in it? If so is it guaranteed its null terminated? It is also always greater then 8?

2080 + if (strstr (sysconfProfileBasename, ".ini"))
2081 + sysconfProfileBase = strndup (sysconfProfileBasename, strlen (sysconfProfileBasename) - 4);
2082 + else if (strstr (sysconfProfileBasename, ".profile"))
2083 + sysconfProfileBase = strndup (sysconfProfileBasename, strlen (sysconfProfileBasename) - 8);

It seems like its 100% sure that the strlen of sysconfProfileBasename will always be greater than 8 (but just wanted to double check)

review: Needs Fixing

« Back to merge proposal