12:19 p.m.
--
Eduardo Otubo
Software Engineer
Linux Technology Center
IBM Systems & Technology Group
Mobile: +55 19 8135 0885
eotubo(a)linux.vnet.ibm.com
4:25 p.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
6:04 p.m.
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(a)linux.vnet.ibm.com
9:35 a.m.
2010/3/30 Eduardo Otubo <otubo(a)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
12:55 p.m.
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(a)linux.vnet.ibm.com
1:05 p.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
6:17 p.m.
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
8 a.m.
2010/6/2 Eduardo Otubo <otubo(a)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
5318
days inactive
5389
days old
7 comments
3 participants
participants (3)
-
Eduardo Otubo -
Eric Blake -
Matthias Bolte