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

[Dan Smith]

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.

>> +        CMSetNameSpace(op, NAMESPACE(ref));

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 at us.ibm.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 188 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvirt-cim/attachments/20080227/b69168b7/attachment.sig>