[Libvirt-cim] [PATCH 2 of 3] Make alloc_cap_instances() available for external use by EC
Heidi Eckhart
heidieck at linux.vnet.ibm.com
Wed Jan 16 12:13:27 UTC 2008
Dan Smith wrote:
> HE> Mhh, as we have many free operations now that can handle NULL
> HE> pointers, I suggest to also make inst_list_free() able to handle
> HE> NULL pointers.
>
> But they're not NULL pointers at that point, the list.list is a
> garbage pointer. Unless you define them as:
>
> struct inst_list list = {NULL, 0, 0};
>
> Otherwise, the pointer will be garbage and a free() will smash the
> heap.
>
> The inst_list_init() call doesn't allocate any memory, it just
> initializes the contents of the struct, so why not always do that
> before proceeding?
>
Ok, thank you for this clarification.
> HE> Agreed. I will cook up a patch for inst_list_free(). The intention
> HE> to move the inst_list_init() functions below
> HE> connect_by_classname() was to avoid some cycles, because the case
> HE> where the provider exits right after provider_is_responsible() or
> HE> connect_by_classname() can happen often.
>
> I think this optimization is likely lost in the noise of the roaring
> CIMOM above us, so I'm not sure it's all that important. However, a
> macro could help make this cleaner I think. Something like what they
> do in the kernel might be helpful:
>
> #define DECLARE_INST_LIST(x) struct inst_list x = {NULL, 0, 0}
>
> so that we can just do this in a function to have pre-initialized
> lists and avoid the inst_list_init() calls:
>
> int function_foo(...) {
> int a;
> char b;
> DECLARE_INST_LIST(list_one);
> DECLARE_INST_LIST(list_two);
>
> ...
> }
>
> I still question the value of this optimization, as I think the
> compiler will inline the inst_list_init() function. At that point,
> this:
>
> struct inst_list foo = {NULL, 0, 0};
>
> surely becomes the same code as this:
>
> struct inst_list foo;
>
> foo.list = NULL;
> foo.max = foo.cur = 0;
>
> Anyone else have thoughts on this?
>
Your explanation is absolutely clear and shows me, that I was nitpickier
than a nitpicker ;). I can only agree that the "optimization" will get
"lost in the noise of the roaring CIMOM above us". I like the suggestion
of the DECLARE macro, but understand, that we have more important things
to do and fix. So skipping it.
> HE> Agreed. I saw that we already have a task for "Fix up all the
> HE> error codes in returned status values (everything is currently
> HE> CMPI_RC_ERR_FAILED)". So we should leave this discussion for
> HE> there. I will remove this change.
>
> Yes, we definitely have work to do on our return codes and error
> messages, but I'd rather address them as a group than sprinkle them in
> with other changes :)
>
> Thanks!
>
>
--
Regards
Heidi Eckhart
Software Engineer
IBM Linux Technology Center - Open Hypervisor
More information about the Libvirt-cim
mailing list