[Libvir] Patch: Fix documentation and code of virGetDomain function
It turns out that you _can't_ pass name=NULL to virGetDomain, despite what the docs say. Rich. -- Emerging Technologies, Red Hat http://et.redhat.com/~rjones/ 64 Baker Street, London, W1U 7DF Mobile: +44 7866 314 421 "[Negative numbers] darken the very whole doctrines of the equations and make dark of the things which are in their nature excessively obvious and simple" (Francis Maseres FRS, mathematician, 1759)
On Wed, 2007-02-28 at 16:24 +0000, Richard W.M. Jones wrote:
virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid) { virDomainPtr ret = NULL;
- if ((!VIR_IS_CONNECT(conn)) || ((name == NULL) && (uuid == NULL)) || + if ((!VIR_IS_CONNECT(conn)) || (name == NULL) || (uuid == NULL) ||
Need the same fix in virGetNetwork() (And DV obviously has a parenthesis fetish ... I don't know what's wrong with: if (!VIR_IS_CONNECT(conn) || !name || !uuid || !conn->hashes->mux) :-) Cheers, Mark.
Mark McLoughlin <markmc@redhat.com> wrote:
On Wed, 2007-02-28 at 16:24 +0000, Richard W.M. Jones wrote:
virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid) { virDomainPtr ret = NULL;
- if ((!VIR_IS_CONNECT(conn)) || ((name == NULL) && (uuid == NULL)) || + if ((!VIR_IS_CONNECT(conn)) || (name == NULL) || (uuid == NULL) ||
Need the same fix in virGetNetwork()
(And DV obviously has a parenthesis fetish ... I don't know what's wrong with:
if (!VIR_IS_CONNECT(conn) || !name || !uuid || !conn->hashes->mux)
:-)
Heh :-) Though using 'uuid == NULL' can be seen as more readable than "!uuid" in that it tells the reader "uuid" is not an integer. In Coreutils, I use this rule to keep at least one class of useless parens at bay: # Avoid useless parentheses like those in this example: # #if defined (SYMBOL) || defined (SYM2) sc_useless_cpp_parens: @grep -n '^# *if .*defined *(' $$($(CVS_LIST_EXCEPT)) && \ { echo '$(ME): found useless parentheses in cpp directive' \ 1>&2; exit 1; } || :
On Thu, Mar 01, 2007 at 09:46:11AM +0000, Mark McLoughlin wrote:
On Wed, 2007-02-28 at 16:24 +0000, Richard W.M. Jones wrote:
virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid) { virDomainPtr ret = NULL;
- if ((!VIR_IS_CONNECT(conn)) || ((name == NULL) && (uuid == NULL)) || + if ((!VIR_IS_CONNECT(conn)) || (name == NULL) || (uuid == NULL) ||
Need the same fix in virGetNetwork()
Applied Rich's patch to CVS along with the corresponding fix to virGetNetwork Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On Tue, Mar 06, 2007 at 10:06:32PM +0000, Daniel P. Berrange wrote:
On Thu, Mar 01, 2007 at 09:46:11AM +0000, Mark McLoughlin wrote:
On Wed, 2007-02-28 at 16:24 +0000, Richard W.M. Jones wrote:
virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid) { virDomainPtr ret = NULL;
- if ((!VIR_IS_CONNECT(conn)) || ((name == NULL) && (uuid == NULL)) || + if ((!VIR_IS_CONNECT(conn)) || (name == NULL) || (uuid == NULL) ||
Need the same fix in virGetNetwork()
Applied Rich's patch to CVS along with the corresponding fix to virGetNetwork
Changing documented semantic because there is a bug (or rather TODO) doesn't sound right to me. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Wed, Mar 07, 2007 at 04:29:16AM -0500, Daniel Veillard wrote:
On Tue, Mar 06, 2007 at 10:06:32PM +0000, Daniel P. Berrange wrote:
On Thu, Mar 01, 2007 at 09:46:11AM +0000, Mark McLoughlin wrote:
On Wed, 2007-02-28 at 16:24 +0000, Richard W.M. Jones wrote:
virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid) { virDomainPtr ret = NULL;
- if ((!VIR_IS_CONNECT(conn)) || ((name == NULL) && (uuid == NULL)) || + if ((!VIR_IS_CONNECT(conn)) || (name == NULL) || (uuid == NULL) ||
Need the same fix in virGetNetwork()
Applied Rich's patch to CVS along with the corresponding fix to virGetNetwork
Changing documented semantic because there is a bug (or rather TODO) doesn't sound right to me.
Well fixing the code to hash based on UUIDs was not so straightforward so the easiest was to update the docs to match reality. All of the current callers always provide a non-NULL name & UUID. When someone gets time we can still make it hash based on UUID instead, but until its done I think its better to have acccurate docs to avoid confusing people using it and then wondering why they get a SEGV on a NULL dereference. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On Wed, Feb 28, 2007 at 04:24:10PM +0000, Richard W.M. Jones wrote:
It turns out that you _can't_ pass name=NULL to virGetDomain, despite what the docs say.
The goal was to allow searching by name or uuid (or both), that was the principle of the API, unfortunately UUID search was not implemented. The right fix is to add the lookup by UUID, as the TODO below indicated /* TODO search by UUID first as they are better differenciators */ changing the API instead is not the right approach. In a nutshell I disagree with the patch, it does do the right thing. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
participants (5)
-
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering -
Mark McLoughlin -
Richard W.M. Jones