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

HE> 2008 ;) Whoops, thanks :) 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. 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 :) HE> FYI - I encountered some problems with another provider, where I HE> tried virDomainFree() with a not initialized dom. This could also HE> happen here with the goto out from conn. Maybe virDomainFree() can HE> not handle NULL ? No, it can handle it just fine. We do this in many, many places elsewhere in the code. I think you might get a warning out of it in some cases, but the code does check for NULL. HE> This step is not necessary as NewObjectPath already sets the HE> namespace. It doesn't for me. If I don't do this, the CBCreateInstance fails with "Target namespace does not exist". We do this in the migration job creation code as well. Er, hmm, maybe it's this one. HE> This should also be not necessary. But as you debug the ref right HE> afterwards ... did you encounter some problems here ? Yes, removing at least one of these results in the behavior described above. I'll revisit and see which is the necessary one. HE> Its more my personal view, but as you "goto out" each time the HE> status is something else than CMPI_RC_OK, it should still be HE> CMPI_RC_OK at this point, if you initialize it right at the HE> beginning of the function with CMPIStatus s = {CMPI_RC_OK, NULL}; HE> . The reason I don't like doing this in all cases is because it's implied success at the bottom of the function, instead of making it explicit. Also, if we are in the habit of doing this, and then have a function where we catch s.rc != CMPI_RC_OK somewhere and take a default value, then we might fall through here and return an error when we don't intend to. However, I've changed it in this case. 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. HE> You also need to do a CMReturnData for a failed request. Ah, yes, thanks. HE> I miss the instance provider interface. Do you plan to add this ? Yes :) -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms(a)us.ibm.com