[Libvirt-cim] [PATCH 1 of 3] Add VirtualSystemSnapshotService

[Heidi Eckhart]

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