Re: [Libvirt-cim] [PATCH 1 of 3] Add VirtualSystemSnapshotService
Dan Smith wrote:
HE> Please can you explain why you set the status here and
overwrite
HE> it right afterwards ? I think the CIM_JOBSTATE is already COMPLETE
HE> here.
I wanted this function to serve three roles: save, save-then-restore,
and restore. In the save-then-restore scenario, we want a status
update after the save is complete, of course.
Ok, thanks - makes sense now for
me.
Because I like
symmetric code, I added the same sort of status update to the end of
restore. In the save-only case, this comes out to setting the save
status and then changing it to "complete" right afterwards.
I don't know if this is a big deal or not, but I think that the code
to make it avoid the extra set would be less pretty. Suggestions are
welcome, of course :)
Well, I have no suggestion to reorder the code - as it would end up in
less pretty code (as you already said). But maybe a little description
to do_snapshot() would help to understand it better.
...
Yes, removing at least one of these results in the behavior described
above. I'll revisit and see which is the necessary one.
Please check if the CMPIInstance contains the namespace information. I
guess this will be the leak.
...
HE> You could use a CMPIString and return its char* to the
HE> caller. This avoids missing frees.
But then I have to pass a broker pointer, since this is used outside
this file. I'll look to see if it makes sense to do that here or
not.
Oh yes, I forgot about that. Mhh, then it might be better to not use
CMPIString, as this would also break the common look and feel with
similar functions in libvirt-cim.
--
Regards
Heidi Eckhart
Software Engineer
IBM Linux Technology Center - Open Hypervisor