[libvirt] [PATCH] Strict check when listing domains
-- Eduardo Otubo Software Engineer Linux Technology Center IBM Systems & Technology Group Mobile: +55 19 8135 0885 eotubo@linux.vnet.ibm.com
On 03/24/2010 11:19 AM, Eduardo Otubo wrote:
/* exit early if there are no domains */ - if (nids == 0) + if (nids_numdomains == 0 || nids_listdomains == 0 + || nids_numdomains != nids_listdomains) return 0;
Should we be reporting an error here...
- if (VIR_ALLOC_N(ids, nids) < 0) { + if (VIR_ALLOC_N(ids, nids_listdomains) < 0) { virReportOOMError(); goto err; }
- if ((nids = phypListDomainsGeneric(conn, ids, nids, 1)) < 0) - goto err; - - /* exit early if there are no domains */ - /* FIXME: phypNumDomainsGeneric() returned > 0 but phypListDomainsGeneric() - * returned 0. indicates this an error condition? - * an even stricter check would be to treat - * - * phypNumDomainsGeneric() != phypListDomainsGeneric() - * - * as an error */
...given this comment? You treated it as an early exit of 0, even if both values were non-zero. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
I am really sorry. I sent an older version of the patch, here is the correct one. Eric Blake wrote:
On 03/24/2010 11:19 AM, Eduardo Otubo wrote:
/* exit early if there are no domains */ - if (nids == 0) + if (nids_numdomains == 0 || nids_listdomains == 0 + || nids_numdomains != nids_listdomains) return 0;
Should we be reporting an error here...
- if (VIR_ALLOC_N(ids, nids) < 0) { + if (VIR_ALLOC_N(ids, nids_listdomains) < 0) { virReportOOMError(); goto err; }
- if ((nids = phypListDomainsGeneric(conn, ids, nids, 1)) < 0) - goto err; - - /* exit early if there are no domains */ - /* FIXME: phypNumDomainsGeneric() returned > 0 but phypListDomainsGeneric() - * returned 0. indicates this an error condition? - * an even stricter check would be to treat - * - * phypNumDomainsGeneric() != phypListDomainsGeneric() - * - * as an error */
...given this comment? You treated it as an early exit of 0, even if both values were non-zero.
-- Eduardo Otubo Software Engineer Linux Technology Center IBM Systems & Technology Group Mobile: +55 19 8135 0885 eotubo@linux.vnet.ibm.com
2010/3/30 Eduardo Otubo <otubo@linux.vnet.ibm.com>:
I am really sorry. I sent an older version of the patch, here is the correct one.
Eric Blake wrote:
On 03/24/2010 11:19 AM, Eduardo Otubo wrote:
/* exit early if there are no domains */ - if (nids == 0) + if (nids_numdomains == 0 || nids_listdomains == 0 + || nids_numdomains != nids_listdomains) return 0;
Should we be reporting an error here...
- if (VIR_ALLOC_N(ids, nids) < 0) { + if (VIR_ALLOC_N(ids, nids_listdomains) < 0) { virReportOOMError(); goto err; } - if ((nids = phypListDomainsGeneric(conn, ids, nids, 1)) < 0) - goto err; - - /* exit early if there are no domains */ - /* FIXME: phypNumDomainsGeneric() returned > 0 but phypListDomainsGeneric() - * returned 0. indicates this an error condition? - * an even stricter check would be to treat - * - * phypNumDomainsGeneric() != phypListDomainsGeneric() - * - * as an error */
...given this comment? You treated it as an early exit of 0, even if both values were non-zero.
Sorry for the late review...
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 4f7efdb..eb6be32 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1849,41 +1849,32 @@ phypUUIDTable_Init(virConnectPtr conn) { uuid_tablePtr uuid_table; phyp_driverPtr phyp_driver; - int nids = 0; + int nids_numdomains = 0; + int nids_listdomains = 0; int *ids = NULL; unsigned int i = 0;
- if ((nids = phypNumDomainsGeneric(conn, 2)) < 0) + if ((nids_numdomains = phypNumDomainsGeneric(conn, 2)) < 0) + goto err; + + if ((nids_listdomains = + phypListDomainsGeneric(conn, ids, nids_listdomains, 1)) < 0)
I think you want to call phypListDomainsGeneric(conn, ids, nids_numdomains, 1) instead of phypListDomainsGeneric(conn, ids, nids_listdomains, 1) Also, you have to move the allocation of ids before the call to phypListDomainsGeneric. Otherwise you're calling phypListDomainsGeneric with ids = NULL resulting in a segfault. Because you currently pass nids_listdomains (which is always 0 at this point) to phypListDomainsGeneric you don't trigger the segfault.
goto err;
/* exit early if there are no domains */ - if (nids == 0) + if (nids_numdomains != nids_listdomains)
You should report an error here, otherwise an "unknown error" will be reported by libvirt.
+ goto err; + else if (nids_numdomains == 0 && nids_listdomains == 0) return 0;
Matthias
Sorry for the late review...
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 4f7efdb..eb6be32 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1849,41 +1849,32 @@ phypUUIDTable_Init(virConnectPtr conn) { uuid_tablePtr uuid_table; phyp_driverPtr phyp_driver; - int nids = 0; + int nids_numdomains = 0; + int nids_listdomains = 0; int *ids = NULL; unsigned int i = 0;
- if ((nids = phypNumDomainsGeneric(conn, 2))< 0) + if ((nids_numdomains = phypNumDomainsGeneric(conn, 2))< 0) + goto err; + + if ((nids_listdomains = + phypListDomainsGeneric(conn, ids, nids_listdomains, 1))< 0)
I think you want to call
phypListDomainsGeneric(conn, ids, nids_numdomains, 1)
instead of
phypListDomainsGeneric(conn, ids, nids_listdomains, 1)
Also, you have to move the allocation of ids before the call to phypListDomainsGeneric. Otherwise you're calling phypListDomainsGeneric with ids = NULL resulting in a segfault. Because you currently pass nids_listdomains (which is always 0 at this point) to phypListDomainsGeneric you don't trigger the segfault.
Thanks for the comments, all fixed :-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 8a9c7a6..423c95d 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1857,41 +1857,33 @@ phypUUIDTable_Init(virConnectPtr conn) { uuid_tablePtr uuid_table; phyp_driverPtr phyp_driver; - int nids = 0; + int nids_numdomains = 0; + int nids_listdomains = 0; int *ids = NULL; unsigned int i = 0; - if ((nids = phypNumDomainsGeneric(conn, 2)) < 0) + if ((nids_numdomains = phypNumDomainsGeneric(conn, 2)) < 0) goto err; - /* exit early if there are no domains */ - if (nids == 0) - return 0; - - if (VIR_ALLOC_N(ids, nids) < 0) { + if (VIR_ALLOC_N(ids, nids_numdomains) < 0) { virReportOOMError(); goto err; } - if ((nids = phypListDomainsGeneric(conn, ids, nids, 1)) < 0) + if ((nids_listdomains = + phypListDomainsGeneric(conn, ids, nids_numdomains, 1)) < 0) goto err; /* exit early if there are no domains */ - /* FIXME: phypNumDomainsGeneric() returned > 0 but phypListDomainsGeneric() - * returned 0. indicates this an error condition? - * an even stricter check would be to treat - * - * phypNumDomainsGeneric() != phypListDomainsGeneric() - * - * as an error */ - if (nids == 0) { - VIR_FREE(ids); - return 0; - } + if (nids_numdomains != nids_listdomains){ + VIR_ERROR(_("Unable to determine number of domains.")); + goto err; + }else if (nids_numdomains == 0 && nids_listdomains == 0) + goto exit; phyp_driver = conn->privateData; uuid_table = phyp_driver->uuid_table; - uuid_table->nlpars = nids; + uuid_table->nlpars = nids_listdomains; /* try to get the table from server */ if (phypUUIDTable_Pull(conn) == -1) { -- Eduardo Otubo Software Engineer Linux Technology Center IBM Systems & Technology Group Mobile: +55 19 8135 0885 eotubo@linux.vnet.ibm.com
On 06/01/2010 11:55 AM, Eduardo Otubo wrote:
Sorry for the late review...
Thanks for the comments, all fixed :-)
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 8a9c7a6..423c95d 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c
Would you mind sending the entire patch, including the git commit message, rather than just the 'git diff' output? That way, your name will automatically show up in the commit message. In this regards, 'git send-email' can be quite a powerful tool, as it takes care of these details.
- if (nids == 0) { - VIR_FREE(ids); - return 0; - } + if (nids_numdomains != nids_listdomains){
Style: use ') {' (like what you just deleted) rather than '){'.
+ VIR_ERROR(_("Unable to determine number of domains."));
s/ERROR/ERROR0/ since you didn't use % or extra args.
+ goto err; + }else if (nids_numdomains == 0 && nids_listdomains == 0)
more style: '} else'. But here, the recent changes to HACKING recommend that you either: if (!cond) goto err; else { ... } or: if (cond) { ... } else { goto err; } That is, if you have a one-line else clause, it should either be the first clause encountered, or it should be in {} to match all the other clauses. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Now sending patches using format-patch and send-email, pretty interesting! This is the final version, fixing the style according to the new HACKING file. --- src/phyp/phyp_driver.c | 34 ++++++++++++++-------------------- 1 files changed, 14 insertions(+), 20 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 8a9c7a6..f8bea42 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1857,41 +1857,34 @@ phypUUIDTable_Init(virConnectPtr conn) { uuid_tablePtr uuid_table; phyp_driverPtr phyp_driver; - int nids = 0; + int nids_numdomains = 0; + int nids_listdomains = 0; int *ids = NULL; unsigned int i = 0; - if ((nids = phypNumDomainsGeneric(conn, 2)) < 0) + if ((nids_numdomains = phypNumDomainsGeneric(conn, 2)) < 0) goto err; - /* exit early if there are no domains */ - if (nids == 0) - return 0; - - if (VIR_ALLOC_N(ids, nids) < 0) { + if (VIR_ALLOC_N(ids, nids_numdomains) < 0) { virReportOOMError(); goto err; } - if ((nids = phypListDomainsGeneric(conn, ids, nids, 1)) < 0) + if ((nids_listdomains = + phypListDomainsGeneric(conn, ids, nids_numdomains, 1)) < 0) goto err; /* exit early if there are no domains */ - /* FIXME: phypNumDomainsGeneric() returned > 0 but phypListDomainsGeneric() - * returned 0. indicates this an error condition? - * an even stricter check would be to treat - * - * phypNumDomainsGeneric() != phypListDomainsGeneric() - * - * as an error */ - if (nids == 0) { - VIR_FREE(ids); - return 0; + if (nids_numdomains == 0 && nids_listdomains == 0) + goto exit; + else if (nids_numdomains != nids_listdomains) { + VIR_ERROR0(_("Unable to determine number of domains.")); + goto err; } phyp_driver = conn->privateData; uuid_table = phyp_driver->uuid_table; - uuid_table->nlpars = nids; + uuid_table->nlpars = nids_listdomains; /* try to get the table from server */ if (phypUUIDTable_Pull(conn) == -1) { @@ -1905,7 +1898,8 @@ phypUUIDTable_Init(virConnectPtr conn) uuid_table->lpars[i]->id = ids[i]; if (virUUIDGenerate(uuid_table->lpars[i]->uuid) < 0) - VIR_WARN("Unable to generate UUID for domain %d", ids[i]); + VIR_WARN("Unable to generate UUID for domain %d", + ids[i]); } } else { virReportOOMError(); -- 1.7.0.4
2010/6/2 Eduardo Otubo <otubo@linux.vnet.ibm.com>:
Now sending patches using format-patch and send-email, pretty interesting! This is the final version, fixing the style according to the new HACKING file.
--- src/phyp/phyp_driver.c | 34 ++++++++++++++-------------------- 1 files changed, 14 insertions(+), 20 deletions(-)
ACK. I pushed this version now. Matthias
participants (3)
-
Eduardo Otubo -
Eric Blake -
Matthias Bolte