Re: [Libvirt-cim] [PATCH V4] DevicePool, reimplement get_diskpool_config with libvirt
Since I don't have an email to reply-to, here is the link: https://www.redhat.com/archives/libvirt-cim/2012-December/msg00039.html In get_diskpool_config(): * Rather than use the "racy" NumOfStoragePools and ListStoragePools, why not use virConnectListAllStoragePools() passing the "Active" flag for all active pools only? * You may even want to consider keeping the returned virStoragePoolPtr structures around.. I also imagine the other objects (networks and domains) could use the similar calls. I guess the answer somewhat depends on what is the minimum version of libvirt that needs to be supported. But if you "have" to stay with the current model... * The return 'names[i]' is something you'd have to free() anyway, so rather than strdup(names[i]), just take it when setting pools[i].tag and set names[i] = NULL; That avoids an error path. John
于 2013-3-12 4:24, John Ferlan 写道:
Since I don't have an email to reply-to, here is the link:
https://www.redhat.com/archives/libvirt-cim/2012-December/msg00039.html
In get_diskpool_config():
* Rather than use the "racy" NumOfStoragePools and ListStoragePools, why not use virConnectListAllStoragePools() passing the "Active" flag for all active pools only?
Let me check if the version of minium libvirt requirement in configure, to see if the API exist.
* You may even want to consider keeping the returned virStoragePoolPtr structures around..
I also imagine the other objects (networks and domains) could use the similar calls. I guess the answer somewhat depends on what is the minimum version of libvirt that needs to be supported.
Yep, that is the problem. To limit the work, I guess other change for networks and domains should be separate patches, if we decide they are worthy.
But if you "have" to stay with the current model...
* The return 'names[i]' is something you'd have to free() anyway, so rather than strdup(names[i]), just take it when setting pools[i].tag and set names[i] = NULL; That avoids an error path.
let me check.
John
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
-- Best Regards Wenchao Xia
于 2013-3-12 13:57, Wenchao Xia 写道:
于 2013-3-12 4:24, John Ferlan 写道:
Since I don't have an email to reply-to, here is the link:
https://www.redhat.com/archives/libvirt-cim/2012-December/msg00039.html
In get_diskpool_config():
* Rather than use the "racy" NumOfStoragePools and ListStoragePools, why not use virConnectListAllStoragePools() passing the "Active" flag for all active pools only?
Let me check if the version of minium libvirt requirement in configure, to see if the API exist.
* You may even want to consider keeping the returned virStoragePoolPtr structures around..
I also imagine the other objects (networks and domains) could use the similar calls. I guess the answer somewhat depends on what is the minimum version of libvirt that needs to be supported.
Yep, that is the problem. To limit the work, I guess other change for networks and domains should be separate patches, if we decide they are worthy.
But if you "have" to stay with the current model... Checked with the new API, it is too new. I hope to stick to API requirement around libvirt 0.9.0 to avoid trouble when user want to just upgrade libvirt-cim as fix, so if this function can ensure no more risk with old libvirt API, I prefer the old way.
* The return 'names[i]' is something you'd have to free() anyway, so rather than strdup(names[i]), just take it when setting pools[i].tag and set names[i] = NULL; That avoids an error path.
let me check.
This seems workable, thanks.
John
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
-- Best Regards Wenchao Xia
participants (2)
-
John Ferlan -
Wenchao Xia