This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/875/ |
On September 19th, 2013, 5:47 a.m. UTC, Jan Synacek wrote:
src/journald/LMI_JournalLogRecordProvider.c (Diff revision 1) 169 get_string_property(const char *property_name, const CMPIInstance* ci)There is a very similar function in the logicalfile provider. Maybe we should move it to the libcommon.
Good idea, will do that in separate commits. Since my journald branch is not up-to-date and there were changes in master, I'll need to rebase and align my code first. But since there are several patches currently on review, let's do this as soon as they're finished.
On September 19th, 2013, 5:47 a.m. UTC, Jan Synacek wrote:
src/journald/LMI_JournalLogRecordProvider.c (Diff revision 1) 175 if (rc.msg != NULL || (d.state & CMPI_nullValue) == CMPI_nullValue ||My gut feeling here is that it's better to check for the rc than for the message. if (rc.rc ...) or more explicit and readable ir (rc.rc != CMPI_RC_OK ...)
Right, an old habit from sblim provider code...
On September 19th, 2013, 5:47 a.m. UTC, Jan Synacek wrote:
src/journald/LMI_JournalLogRecordProvider.c (Diff revision 1) 226 if (r < 0) {227 sd_journal_close(journal);228 KReturn2(_cb, ERR_FAILED, "Error seeking to the end of the journal: %s\n", strerror(-r));229 }
These pieces of code are fairly common throughout the code. Wrap in a macro / make a function?
Even though I'm not fan of local custom defines as it looks horribly disassociated to the rest of the code, I admit the current code looks horrible the same way. Moved to macro for the time being, until I decide it looks horrible again :-)
On September 19th, 2013, 5:47 a.m. UTC, Jan Synacek wrote:
s/fiull/full/ in the description.
Nice catch, thx.
- Tomáš
On September 18th, 2013, 3:22 p.m. UTC, Tomáš Bžatek wrote:
Review request for OpenLMI Developers.
By Tomáš Bžatek.
Updated Sept. 18, 2013, 3:22 p.m.
Repository:
openlmi-providers
Description
Diffs
|