-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-openlmi.rhcloud.com/r/1289/#review1799
-----------------------------------------------------------
src/service-dbus/LMI_ServiceInstanceModificationIndicationProvider.c
<
http://reviewboard-openlmi.rhcloud.com/r/1289/#comment1032>
Preferably destroy local data after indications have been stopped, this is rather racy
as long as the thread may be still using the memory.
(This is wrong also in account, will fix that right away. Thanks for pointing this out
:-))
src/service-dbus/util/serviceutil.c
<
http://reviewboard-openlmi.rhcloud.com/r/1289/#comment1033>
This is a little racy with ind_destroy() destroying the mainloop. Mainloop creating
does not need to be here, should be fine in ind_init() before the thread creation.
src/service-dbus/util/serviceutil.c
<
http://reviewboard-openlmi.rhcloud.com/r/1289/#comment1034>
Useless assignment.
src/service-dbus/util/serviceutil.c
<
http://reviewboard-openlmi.rhcloud.com/r/1289/#comment1035>
Leaking si->context, also on some more returns below.
src/service-dbus/util/serviceutil.c
<
http://reviewboard-openlmi.rhcloud.com/r/1289/#comment1036>
Also not needed here as every (error != NULL) case ends with return and error is thus
still NULL.
Generally, this is a way for memleaks. g_error_set() can handle non-NULL error just
fine, printing an assert and continuing further. That way you can easily discover
programming errors.
src/service-dbus/util/serviceutil.c
<
http://reviewboard-openlmi.rhcloud.com/r/1289/#comment1038>
Free the result with g_variant_unref().
src/service-dbus/util/serviceutil.c
<
http://reviewboard-openlmi.rhcloud.com/r/1289/#comment1037>
I think the tmps would need to be free'd. Alternatively, changing the GVariant
format string could give you const gchar* instead (see the "&" modifier:
https://developer.gnome.org/glib/stable/gvariant-format-strings.html#gvar...
)
src/service-dbus/util/serviceutil.c
<
http://reviewboard-openlmi.rhcloud.com/r/1289/#comment1031>
This is no longer needed, see recently introduced im_register_filter_classes() method:
https://git.fedorahosted.org/cgit/openlmi-providers.git/commit/?id=cb1ffc...
src/service-dbus/util/serviceutil.c
<
http://reviewboard-openlmi.rhcloud.com/r/1289/#comment1039>
Hint: Preferably wait for the thread to end gracefully before unrefing the mainloop
(see pthread_join())
src/service-dbus/util/serviceutil.c
<
http://reviewboard-openlmi.rhcloud.com/r/1289/#comment1040>
Testing for si->manager_proxy to exist but using it for a call before...
Several minor issues found, otherwise the code looks good.
- Tomáš Bžatek
On Nov. 26, 2013, 9:58 a.m., Vitezslav Crhonek wrote:
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-openlmi.rhcloud.com/r/1289/
-----------------------------------------------------------
(Updated Nov. 26, 2013, 9:58 a.m.)
Review request for OpenLMI Developers.
Repository: openlmi-providers
Description
-------
Service: Support indications on instance property modification.
Diffs
-----
mof/60_LMI_Service.mof 1a86fec8462402d5f5b5ff4f2661596dca470433
src/service-dbus/CMakeLists.txt 3cc2082ac2b28660b854c5e81a3eb581bac8efb5
src/service-dbus/LMI_ServiceInstanceModificationIndicationProvider.c PRE-CREATION
src/service-dbus/util/serviceutil.h fbdadad5be2d37920407783d806d36af476d62cb
src/service-dbus/util/serviceutil.c 50b18b91cd3083a821a6a3be5f682aafc509be3d
Diff:
http://reviewboard-openlmi.rhcloud.com/r/1289/diff/
Testing
-------
Thanks,
Vitezslav Crhonek