[libvirt] [PATCH] provide the object name on lookup error
There were a number of places where the object name was not not reported, based on the use I don't think the string could be NULL in any of those. I also fixed a few POOL error which were really VOLume ones. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Tue, Jul 07, 2009 at 05:52:03PM +0200, Daniel Veillard wrote:
There were a number of places where the object name was not not reported, based on the use I don't think the string could be NULL in any of those. I also fixed a few POOL error which were really VOLume ones.
There's a few places here still using VIR_ERR_INVALID_xxxxx which could also be changed to VIR_ERR_NO_xxxx, since the INVALID errors codes description is refering to bad pointer addresses. Aside from that, this looks good. Regards, Daniel
diff --git a/src/network_driver.c b/src/network_driver.c index c5c4c2d..9621782 100644 --- a/src/network_driver.c +++ b/src/network_driver.c @@ -971,7 +971,7 @@ static virNetworkPtr networkLookupByName(virConnectPtr conn, networkDriverUnlock(driver); if (!network) { networkReportError(conn, NULL, NULL, VIR_ERR_NO_NETWORK, - "%s", _("no network with matching name")); + _("no network with matching name '%s'"), name); goto cleanup; }
diff --git a/src/node_device.c b/src/node_device.c index e0aaefc..74e7a4f 100644 --- a/src/node_device.c +++ b/src/node_device.c @@ -280,7 +280,8 @@ static char *nodeDeviceDumpXML(virNodeDevicePtr dev,
if (!obj) { virNodeDeviceReportError(dev->conn, VIR_ERR_INVALID_NODE_DEVICE, - "%s", _("no node device with matching name")); + _("no node device with matching name '%s'"), + dev->name); goto cleanup; }
@@ -308,7 +309,8 @@ static char *nodeDeviceGetParent(virNodeDevicePtr dev)
if (!obj) { virNodeDeviceReportError(dev->conn, VIR_ERR_INVALID_NODE_DEVICE, - "%s", _("no node device with matching name")); + _("no node device with matching name '%s'"), + dev->name); goto cleanup; }
@@ -342,7 +344,8 @@ static int nodeDeviceNumOfCaps(virNodeDevicePtr dev)
if (!obj) { virNodeDeviceReportError(dev->conn, VIR_ERR_INVALID_NODE_DEVICE, - "%s", _("no node device with matching name")); + _("no node device with matching name '%s'"), + dev->name); goto cleanup; }
@@ -372,7 +375,8 @@ nodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames)
if (!obj) { virNodeDeviceReportError(dev->conn, VIR_ERR_INVALID_NODE_DEVICE, - "%s", _("no node device with matching name")); + _("no node device with matching name '%s'"), + dev->name); goto cleanup; }
diff --git a/src/storage_driver.c b/src/storage_driver.c index 5a26993..49e2c84 100644 --- a/src/storage_driver.c +++ b/src/storage_driver.c @@ -295,7 +295,7 @@ storagePoolLookupByName(virConnectPtr conn,
if (!pool) { virStorageReportError(conn, VIR_ERR_NO_STORAGE_POOL, - "%s", _("no pool with matching name")); + _("no pool with matching name '%s'"), name); goto cleanup; }
@@ -1099,8 +1099,9 @@ storageVolumeLookupByName(virStoragePoolPtr obj, vol = virStorageVolDefFindByName(pool, name);
if (!vol) { - virStorageReportError(obj->conn, VIR_ERR_INVALID_STORAGE_POOL, - "%s", _("no storage vol with matching name")); + virStorageReportError(obj->conn, VIR_ERR_INVALID_STORAGE_VOL, + _("no storage vol with matching name '%s'"), + name); goto cleanup; }
@@ -1340,7 +1341,8 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj,
if (diffpool && !origpool) { virStorageReportError(obj->conn, VIR_ERR_INVALID_STORAGE_POOL, - "%s", _("no storage pool with matching name")); + _("no storage pool with matching name '%s'"), + vobj->pool); goto cleanup; }
@@ -1361,8 +1363,9 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj,
origvol = virStorageVolDefFindByName(origpool, vobj->name); if (!origvol) { - virStorageReportError(obj->conn, VIR_ERR_INVALID_STORAGE_POOL, - "%s", _("no storage vol with matching name")); + virStorageReportError(obj->conn, VIR_ERR_INVALID_STORAGE_VOL, + _("no storage vol with matching name '%s'"), + vobj->name); goto cleanup; }
@@ -1501,8 +1504,9 @@ storageVolumeDelete(virStorageVolPtr obj, vol = virStorageVolDefFindByName(pool, obj->name);
if (!vol) { - virStorageReportError(obj->conn, VIR_ERR_INVALID_STORAGE_POOL, - "%s", _("no storage vol with matching name")); + virStorageReportError(obj->conn, VIR_ERR_INVALID_STORAGE_VOL, + _("no storage vol with matching name '%s'"), + obj->name); goto cleanup; }
@@ -1576,8 +1580,9 @@ storageVolumeGetInfo(virStorageVolPtr obj, vol = virStorageVolDefFindByName(pool, obj->name);
if (!vol) { - virStorageReportError(obj->conn, VIR_ERR_INVALID_STORAGE_POOL, - "%s", _("no storage vol with matching name")); + virStorageReportError(obj->conn, VIR_ERR_INVALID_STORAGE_VOL, + _("no storage vol with matching name '%s'"), + obj->name); goto cleanup; }
@@ -1628,8 +1633,9 @@ storageVolumeGetXMLDesc(virStorageVolPtr obj, vol = virStorageVolDefFindByName(pool, obj->name);
if (!vol) { - virStorageReportError(obj->conn, VIR_ERR_INVALID_STORAGE_POOL, - "%s", _("no storage vol with matching name")); + virStorageReportError(obj->conn, VIR_ERR_INVALID_STORAGE_VOL, + _("no storage vol with matching name '%s'"), + obj->name); goto cleanup; }
@@ -1674,8 +1680,9 @@ storageVolumeGetPath(virStorageVolPtr obj) { vol = virStorageVolDefFindByName(pool, obj->name);
if (!vol) { - virStorageReportError(obj->conn, VIR_ERR_INVALID_STORAGE_POOL, - "%s", _("no storage vol with matching name")); + virStorageReportError(obj->conn, VIR_ERR_INVALID_STORAGE_VOL, + _("no storage vol with matching name '%s'"), + obj->name); goto cleanup; }
-- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote:
On Tue, Jul 07, 2009 at 05:52:03PM +0200, Daniel Veillard wrote:
There were a number of places where the object name was not not reported, based on the use I don't think the string could be NULL in any of those. I also fixed a few POOL error which were really VOLume ones.
There's a few places here still using VIR_ERR_INVALID_xxxxx which could also be changed to VIR_ERR_NO_xxxx, since the INVALID errors codes description is refering to bad pointer addresses.
Aside from that, this looks good.
Kind of a tangent, but... Can we go back to using macros for all this duplicate validation? For example, the idiom: if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(dom->uuid, uuidstr); qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } Takes up over 200 lines in qemu_driver.c. Sure, having a goto in a macro seems disgusting, but label names are used consistently so it hopefully wouldn't cause any problems. Would there be any objections to reintroducing macros for this stuff? Can CIL handle macros when doing the lock checking? - Cole
On Tue, Jul 07, 2009 at 12:52:48PM -0400, Cole Robinson wrote:
Kind of a tangent, but...
Can we go back to using macros for all this duplicate validation? For example, the idiom:
if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(dom->uuid, uuidstr); qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; }
Takes up over 200 lines in qemu_driver.c. Sure, having a goto in a macro seems disgusting, but label names are used consistently so it hopefully wouldn't cause any problems.
I don't really like the use of macros when flow control is involved because I think its important for people reading the code to clearly see code paths. I agree the amount of code involved here is getting a bit cumbersome though, particularly for UUID based errors. Part of the problem here is the tedious conversion from the raw UUID to printable UUID. This is causing us pain elsewhere - we hash virDomainPtr objects based on name since you can't use a raw UUID as a hash key. I think we should do a couple of things: * Change virDomainPtr internal struct to store the printable UUID in canonical format. This would allow us to remove all the char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(dom->uuid, uuidstr); * Add a macro around the qemuReportError() call, specifically for the VIR_ERR_NO_DOMAIN case, in same way we did for VIR_ERR_NO_MEMORY and VIR_ERR_SYSTEM_ERROR. #define virReportNoDomain(dom) \ virReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, _("no domain with matching name '%s' uuid '%s'"), dom->name, dom->uuid) This would make code to report an error look like if (!vm) { virReportNoDomain(dom); goto error; } * Change virConnectPtr/virGetDomain to do hashing based on dom->uuid, for better uniqueness which would avoid some problems we've seen in virt-manager with name based hashing. Further along, I'd also like to see us try to get some more generic helper methods for certain API operations. For example, LXC, QEMU and UML drivers all have very similar code for certain methods which could likely be pulled into a shared module
Would there be any objections to reintroducing macros for this stuff? Can CIL handle macros when doing the lock checking?
CIL works on the intermediate files between the cpp and compile phase, so it doesn't care about macros from a functional POV. The only problem is human interpretation of the error location info it produces, since you'll get the line number of the macro call, not th eexact line within the macro - same problem you see in GDB when debugging code with macros. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Cole Robinson wrote:
Daniel P. Berrange wrote:
On Tue, Jul 07, 2009 at 05:52:03PM +0200, Daniel Veillard wrote:
There were a number of places where the object name was not not reported, based on the use I don't think the string could be NULL in any of those. I also fixed a few POOL error which were really VOLume ones. There's a few places here still using VIR_ERR_INVALID_xxxxx which could also be changed to VIR_ERR_NO_xxxx, since the INVALID errors codes description is refering to bad pointer addresses.
Aside from that, this looks good.
Kind of a tangent, but...
Can we go back to using macros for all this duplicate validation? For example, the idiom:
if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(dom->uuid, uuidstr); qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN, _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; }
Takes up over 200 lines in qemu_driver.c. Sure, having a goto in a macro seems disgusting, but label names are used consistently so it hopefully wouldn't cause any problems.
Would there be any objections to reintroducing macros for this stuff? Can CIL handle macros when doing the lock checking?
Yeah, I have to agree with Dan here. Having macros that change code flow is just evil when you are trying to read the code later on. -- Chris Lalancette
On Tue, Jul 07, 2009 at 05:50:58PM +0100, Daniel P. Berrange wrote:
On Tue, Jul 07, 2009 at 05:52:03PM +0200, Daniel Veillard wrote:
There were a number of places where the object name was not not reported, based on the use I don't think the string could be NULL in any of those. I also fixed a few POOL error which were really VOLume ones.
There's a few places here still using VIR_ERR_INVALID_xxxxx which could also be changed to VIR_ERR_NO_xxxx, since the INVALID errors codes description is refering to bad pointer addresses.
Aside from that, this looks good.
Ah, right ! I fixed those too when in scope of the patch, applied and commited, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (4)
-
Chris Lalancette -
Cole Robinson -
Daniel P. Berrange -
Daniel Veillard