10:52 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
11:50 a.m.
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(a)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 :|
11:52 a.m.
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
12:28 p.m.
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 :|
4:03 a.m.
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
5:04 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
5616
days inactive
5617
days old
5 comments
4 participants
participants (4)
-
Chris Lalancette -
Cole Robinson -
Daniel P. Berrange -
Daniel Veillard