[libvirt] [PATCH 0/5] Miscellaneous cleanups
My cleanup branch got up to 5 commits so in order to get rid of it, I'm sending it. Some of them could be most probably pushed as trivial, but since some are older (even though the commit date is new), I'm sending it in the pack together. Martin Kletzander (5): fix typo in the word affinities conf: minor indentation cleanups conf: Don't format cputune element when not needed conf: eliminate redundant use of VIR_ALLOC docs: aesthetical cleanups docs/hacking.html.in | 12 ++++---- src/conf/domain_conf.c | 80 ++++++++++++++++++++++-------------------------- src/libxl/libxl_driver.c | 4 +-- src/qemu/qemu_process.c | 8 ++--- 4 files changed, 48 insertions(+), 56 deletions(-) -- 1.8.0.2
This patch fixes just the word Affinites to Affinities (it's really painful to search in TAGS without being able to find the right function). --- src/libxl/libxl_driver.c | 4 ++-- src/qemu/qemu_process.c | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 302f81c..de31e12 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -609,7 +609,7 @@ error: } static int -libxlDomainSetVcpuAffinites(libxlDriverPrivatePtr driver, virDomainObjPtr vm) +libxlDomainSetVcpuAffinities(libxlDriverPrivatePtr driver, virDomainObjPtr vm) { libxlDomainObjPrivatePtr priv = vm->privateData; virDomainDefPtr def = vm->def; @@ -808,7 +808,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, if (libxlCreateDomEvents(vm) < 0) goto error; - if (libxlDomainSetVcpuAffinites(driver, vm) < 0) + if (libxlDomainSetVcpuAffinities(driver, vm) < 0) goto error; if (!start_paused) { diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index cc0e947..89796af 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2025,7 +2025,7 @@ qemuProcessSetLinkStates(virDomainObjPtr vm) /* Set CPU affinities for vcpus if vcpupin xml provided. */ static int -qemuProcessSetVcpuAffinites(virConnectPtr conn ATTRIBUTE_UNUSED, +qemuProcessSetVcpuAffinities(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; @@ -2058,7 +2058,7 @@ cleanup: /* Set CPU affinities for emulator threads. */ static int -qemuProcessSetEmulatorAffinites(virConnectPtr conn ATTRIBUTE_UNUSED, +qemuProcessSetEmulatorAffinities(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainObjPtr vm) { virBitmapPtr cpumask; @@ -3845,11 +3845,11 @@ int qemuProcessStart(virConnectPtr conn, goto cleanup; VIR_DEBUG("Setting VCPU affinities"); - if (qemuProcessSetVcpuAffinites(conn, vm) < 0) + if (qemuProcessSetVcpuAffinities(conn, vm) < 0) goto cleanup; VIR_DEBUG("Setting affinity of emulator threads"); - if (qemuProcessSetEmulatorAffinites(conn, vm) < 0) + if (qemuProcessSetEmulatorAffinities(conn, vm) < 0) goto cleanup; VIR_DEBUG("Setting any required VM passwords"); -- 1.8.0.2
On 2012年12月17日 23:17, Martin Kletzander wrote:
This patch fixes just the word Affinites to Affinities (it's really painful to search in TAGS without being able to find the right function). --- src/libxl/libxl_driver.c | 4 ++-- src/qemu/qemu_process.c | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 302f81c..de31e12 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -609,7 +609,7 @@ error: }
static int -libxlDomainSetVcpuAffinites(libxlDriverPrivatePtr driver, virDomainObjPtr vm) +libxlDomainSetVcpuAffinities(libxlDriverPrivatePtr driver, virDomainObjPtr vm) { libxlDomainObjPrivatePtr priv = vm->privateData; virDomainDefPtr def = vm->def; @@ -808,7 +808,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, if (libxlCreateDomEvents(vm)< 0) goto error;
- if (libxlDomainSetVcpuAffinites(driver, vm)< 0) + if (libxlDomainSetVcpuAffinities(driver, vm)< 0) goto error;
if (!start_paused) { diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index cc0e947..89796af 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2025,7 +2025,7 @@ qemuProcessSetLinkStates(virDomainObjPtr vm)
/* Set CPU affinities for vcpus if vcpupin xml provided. */ static int -qemuProcessSetVcpuAffinites(virConnectPtr conn ATTRIBUTE_UNUSED, +qemuProcessSetVcpuAffinities(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; @@ -2058,7 +2058,7 @@ cleanup:
/* Set CPU affinities for emulator threads. */ static int -qemuProcessSetEmulatorAffinites(virConnectPtr conn ATTRIBUTE_UNUSED, +qemuProcessSetEmulatorAffinities(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainObjPtr vm) { virBitmapPtr cpumask; @@ -3845,11 +3845,11 @@ int qemuProcessStart(virConnectPtr conn, goto cleanup;
VIR_DEBUG("Setting VCPU affinities"); - if (qemuProcessSetVcpuAffinites(conn, vm)< 0) + if (qemuProcessSetVcpuAffinities(conn, vm)< 0) goto cleanup;
VIR_DEBUG("Setting affinity of emulator threads"); - if (qemuProcessSetEmulatorAffinites(conn, vm)< 0) + if (qemuProcessSetEmulatorAffinities(conn, vm)< 0) goto cleanup;
VIR_DEBUG("Setting any required VM passwords");
ACK
On few places there are too many levels of indentation when some of them can be fixed with negating the option they are in or omitting useless condition altogether. --- src/conf/domain_conf.c | 65 +++++++++++++++++++++++++------------------------- 1 file changed, 32 insertions(+), 33 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 19af058..cba910a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9060,21 +9060,22 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, } for (i = 0; i < def->vcpus; i++) { - if (!virDomainVcpuPinIsDuplicate(def->cputune.vcpupin, - def->cputune.nvcpupin, - i)) { - virDomainVcpuPinDefPtr vcpupin = NULL; + if (virDomainVcpuPinIsDuplicate(def->cputune.vcpupin, + def->cputune.nvcpupin, + i)) + continue; - if (VIR_ALLOC(vcpupin) < 0) { - virReportOOMError(); - goto error; - } + virDomainVcpuPinDefPtr vcpupin = NULL; - vcpupin->cpumask = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN); - virBitmapCopy(vcpupin->cpumask, def->cpumask); - vcpupin->vcpuid = i; - def->cputune.vcpupin[def->cputune.nvcpupin++] = vcpupin; + if (VIR_ALLOC(vcpupin) < 0) { + virReportOOMError(); + goto error; } + + vcpupin->cpumask = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN); + virBitmapCopy(vcpupin->cpumask, def->cpumask); + vcpupin->vcpuid = i; + def->cputune.vcpupin[def->cputune.nvcpupin++] = vcpupin; } } @@ -13921,31 +13922,29 @@ virDomainDefFormatInternal(virDomainDefPtr def, "</emulator_quota>\n", def->cputune.emulator_quota); - if (def->cputune.vcpupin) { - for (i = 0; i < def->cputune.nvcpupin; i++) { - /* Ignore the vcpupin which inherit from "cpuset" - * of "<vcpu>." - */ - if (def->cpumask && - virBitmapEqual(def->cpumask, - def->cputune.vcpupin[i]->cpumask)) - continue; - - virBufferAsprintf(buf, " <vcpupin vcpu='%u' ", - def->cputune.vcpupin[i]->vcpuid); + for (i = 0; i < def->cputune.nvcpupin; i++) { + /* Ignore the vcpupin which inherit from "cpuset" + * of "<vcpu>." + */ + if (def->cpumask && + virBitmapEqual(def->cpumask, + def->cputune.vcpupin[i]->cpumask)) + continue; - char *cpumask = NULL; - cpumask = virBitmapFormat(def->cputune.vcpupin[i]->cpumask); + virBufferAsprintf(buf, " <vcpupin vcpu='%u' ", + def->cputune.vcpupin[i]->vcpuid); - if (cpumask == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("failed to format cpuset for vcpupin")); - goto cleanup; - } + char *cpumask = NULL; + cpumask = virBitmapFormat(def->cputune.vcpupin[i]->cpumask); - virBufferAsprintf(buf, "cpuset='%s'/>\n", cpumask); - VIR_FREE(cpumask); + if (cpumask == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("failed to format cpuset for vcpupin")); + goto cleanup; } + + virBufferAsprintf(buf, "cpuset='%s'/>\n", cpumask); + VIR_FREE(cpumask); } if (def->cputune.emulatorpin) { -- 1.8.0.2
On 2012年12月17日 23:17, Martin Kletzander wrote:
On few places there are too many levels of indentation when some of them can be fixed with negating the option they are in or omitting useless condition altogether. --- src/conf/domain_conf.c | 65 +++++++++++++++++++++++++------------------------- 1 file changed, 32 insertions(+), 33 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 19af058..cba910a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9060,21 +9060,22 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, }
for (i = 0; i< def->vcpus; i++) { - if (!virDomainVcpuPinIsDuplicate(def->cputune.vcpupin, - def->cputune.nvcpupin, - i)) { - virDomainVcpuPinDefPtr vcpupin = NULL; + if (virDomainVcpuPinIsDuplicate(def->cputune.vcpupin, + def->cputune.nvcpupin, + i)) + continue;
- if (VIR_ALLOC(vcpupin)< 0) { - virReportOOMError(); - goto error; - } + virDomainVcpuPinDefPtr vcpupin = NULL;
- vcpupin->cpumask = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN); - virBitmapCopy(vcpupin->cpumask, def->cpumask); - vcpupin->vcpuid = i; - def->cputune.vcpupin[def->cputune.nvcpupin++] = vcpupin; + if (VIR_ALLOC(vcpupin)< 0) { + virReportOOMError(); + goto error; } + + vcpupin->cpumask = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN); + virBitmapCopy(vcpupin->cpumask, def->cpumask); + vcpupin->vcpuid = i; + def->cputune.vcpupin[def->cputune.nvcpupin++] = vcpupin; } }
@@ -13921,31 +13922,29 @@ virDomainDefFormatInternal(virDomainDefPtr def, "</emulator_quota>\n", def->cputune.emulator_quota);
- if (def->cputune.vcpupin) { - for (i = 0; i< def->cputune.nvcpupin; i++) { - /* Ignore the vcpupin which inherit from "cpuset" - * of "<vcpu>." - */ - if (def->cpumask&& - virBitmapEqual(def->cpumask, - def->cputune.vcpupin[i]->cpumask)) - continue; - - virBufferAsprintf(buf, "<vcpupin vcpu='%u' ", - def->cputune.vcpupin[i]->vcpuid); + for (i = 0; i< def->cputune.nvcpupin; i++) { + /* Ignore the vcpupin which inherit from "cpuset" + * of "<vcpu>." + */ + if (def->cpumask&& + virBitmapEqual(def->cpumask, + def->cputune.vcpupin[i]->cpumask)) + continue;
- char *cpumask = NULL; - cpumask = virBitmapFormat(def->cputune.vcpupin[i]->cpumask); + virBufferAsprintf(buf, "<vcpupin vcpu='%u' ", + def->cputune.vcpupin[i]->vcpuid);
- if (cpumask == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("failed to format cpuset for vcpupin")); - goto cleanup; - } + char *cpumask = NULL; + cpumask = virBitmapFormat(def->cputune.vcpupin[i]->cpumask);
- virBufferAsprintf(buf, "cpuset='%s'/>\n", cpumask); - VIR_FREE(cpumask); + if (cpumask == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("failed to format cpuset for vcpupin")); + goto cleanup; } + + virBufferAsprintf(buf, "cpuset='%s'/>\n", cpumask); + VIR_FREE(cpumask); }
if (def->cputune.emulatorpin) {
ACK
Commit 60b176c3d0f0d5037acfa5e27c7753f657833a0b introduced a bug that when editing an XML with cputune similar to this: ... <vcpu placement='static' current='1'>2</vcpu> <cputune> <vcpupin vcpu="1" cpuset="0"/> </cputune> ... results in formatted XML that looks like this: ... <vcpu placement='static' current='1'>2</vcpu> <cputune> </cputune> ... That is caused by a condition depending on def->cputune.vcpupin being set rather than checking def->cputune.nvcpupin. Notice that nvcpupin can be 0 and vcpupin can still be allocated since it's a pointer to an array, so no harm done there. --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index cba910a..329ada3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13896,7 +13896,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAsprintf(buf, ">%u</vcpu>\n", def->maxvcpus); if (def->cputune.shares || - (def->cputune.vcpupin && !virDomainIsAllVcpupinInherited(def)) || + (def->cputune.nvcpupin && !virDomainIsAllVcpupinInherited(def)) || def->cputune.period || def->cputune.quota || def->cputune.emulatorpin || def->cputune.emulator_period || def->cputune.emulator_quota) -- 1.8.0.2
On 2012年12月17日 23:17, Martin Kletzander wrote:
Commit 60b176c3d0f0d5037acfa5e27c7753f657833a0b introduced a bug that when editing an XML with cputune similar to this:
Thanks for fixing this.
... <vcpu placement='static' current='1'>2</vcpu> <cputune> <vcpupin vcpu="1" cpuset="0"/> </cputune> ...
results in formatted XML that looks like this:
... <vcpu placement='static' current='1'>2</vcpu> <cputune> </cputune> ...
That is caused by a condition depending on def->cputune.vcpupin being set rather than checking def->cputune.nvcpupin. Notice that nvcpupin can be 0 and vcpupin can still be allocated since it's a pointer to an array, so no harm done there. --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index cba910a..329ada3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13896,7 +13896,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAsprintf(buf, ">%u</vcpu>\n", def->maxvcpus);
if (def->cputune.shares || - (def->cputune.vcpupin&& !virDomainIsAllVcpupinInherited(def)) || + (def->cputune.nvcpupin&& !virDomainIsAllVcpupinInherited(def)) ||
This is a good fix, but I don't think it fixes the problem what commit message describes. As both def->cputune.vcpupin and def->cputune.nvcpupin should be true here for the testing case you wrote in the commit message. And as long as we try to use nvcpupin here, we should use nvcpupin when formating "</cputune>" too. Osier
On 12/18/2012 04:01 AM, Osier Yang wrote:
On 2012年12月17日 23:17, Martin Kletzander wrote:
Commit 60b176c3d0f0d5037acfa5e27c7753f657833a0b introduced a bug that when editing an XML with cputune similar to this:
Thanks for fixing this.
... <vcpu placement='static' current='1'>2</vcpu> <cputune> <vcpupin vcpu="1" cpuset="0"/> </cputune> ...
results in formatted XML that looks like this:
... <vcpu placement='static' current='1'>2</vcpu> <cputune> </cputune> ...
That is caused by a condition depending on def->cputune.vcpupin being set rather than checking def->cputune.nvcpupin. Notice that nvcpupin can be 0 and vcpupin can still be allocated since it's a pointer to an array, so no harm done there. --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index cba910a..329ada3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13896,7 +13896,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAsprintf(buf, ">%u</vcpu>\n", def->maxvcpus);
if (def->cputune.shares || - (def->cputune.vcpupin&& !virDomainIsAllVcpupinInherited(def)) || + (def->cputune.nvcpupin&& !virDomainIsAllVcpupinInherited(def)) ||
This is a good fix, but I don't think it fixes the problem what commit message describes. As both def->cputune.vcpupin and def->cputune.nvcpupin should be true here for the testing case you wrote in the commit message.
I checked this fixes it (the vcpupin is allocated, but nvcpupin is 0, which means no values are in vcpupin), but I'll check the other places where we format it as well.
And as long as we try to use nvcpupin here, we should use nvcpupin when formating "</cputune>" too.
Osier
On 2012年12月18日 19:20, Martin Kletzander wrote:
On 12/18/2012 04:01 AM, Osier Yang wrote:
On 2012年12月17日 23:17, Martin Kletzander wrote:
Commit 60b176c3d0f0d5037acfa5e27c7753f657833a0b introduced a bug that when editing an XML with cputune similar to this:
Thanks for fixing this.
... <vcpu placement='static' current='1'>2</vcpu> <cputune> <vcpupin vcpu="1" cpuset="0"/> </cputune> ...
results in formatted XML that looks like this:
... <vcpu placement='static' current='1'>2</vcpu> <cputune> </cputune> ...
That is caused by a condition depending on def->cputune.vcpupin being set rather than checking def->cputune.nvcpupin. Notice that nvcpupin can be 0 and vcpupin can still be allocated since it's a pointer to an array, so no harm done there. --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index cba910a..329ada3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13896,7 +13896,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAsprintf(buf, ">%u</vcpu>\n", def->maxvcpus);
if (def->cputune.shares || - (def->cputune.vcpupin&& !virDomainIsAllVcpupinInherited(def)) || + (def->cputune.nvcpupin&& !virDomainIsAllVcpupinInherited(def)) ||
This is a good fix, but I don't think it fixes the problem what commit message describes. As both def->cputune.vcpupin and def->cputune.nvcpupin should be true here for the testing case you wrote in the commit message.
I checked this fixes it (the vcpupin is allocated, but nvcpupin is 0, which means no values are in vcpupin),
It sounds like a bug, as nvcpupin should not be 0, per there is one specified in the domain config.
but I'll check the other places where we format it as well.
And as long as we try to use nvcpupin here, we should use nvcpupin when formating "</cputune>" too.
Osier
On 12/18/2012 12:39 PM, Osier Yang wrote:
On 2012年12月18日 19:20, Martin Kletzander wrote:
On 12/18/2012 04:01 AM, Osier Yang wrote:
On 2012年12月17日 23:17, Martin Kletzander wrote:
Commit 60b176c3d0f0d5037acfa5e27c7753f657833a0b introduced a bug that when editing an XML with cputune similar to this:
Thanks for fixing this.
... <vcpu placement='static' current='1'>2</vcpu> <cputune> <vcpupin vcpu="1" cpuset="0"/> </cputune> ...
results in formatted XML that looks like this:
... <vcpu placement='static' current='1'>2</vcpu> <cputune> </cputune> ...
That is caused by a condition depending on def->cputune.vcpupin being set rather than checking def->cputune.nvcpupin. Notice that nvcpupin can be 0 and vcpupin can still be allocated since it's a pointer to an array, so no harm done there. --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index cba910a..329ada3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13896,7 +13896,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAsprintf(buf, ">%u</vcpu>\n", def->maxvcpus);
if (def->cputune.shares || - (def->cputune.vcpupin&& !virDomainIsAllVcpupinInherited(def)) || + (def->cputune.nvcpupin&& !virDomainIsAllVcpupinInherited(def)) ||
This is a good fix, but I don't think it fixes the problem what commit message describes. As both def->cputune.vcpupin and def->cputune.nvcpupin should be true here for the testing case you wrote in the commit message.
I checked this fixes it (the vcpupin is allocated, but nvcpupin is 0, which means no values are in vcpupin),
It sounds like a bug, as nvcpupin should not be 0, per there is one specified in the domain config.
Actually, no. It is in the way we agreed to parse those parameters. For each pin the code does: if (vcpupin->vcpuid >= def->vcpus) /* To avoid the regression when daemon loading * domain confs, we can't simply error out if * <vcpupin> nodes greater than current vcpus, * ignoring them instead. */ VIR_WARN("Ignore vcpupin for not onlined vcpus"); else def->cputune.vcpupin[def->cputune.nvcpupin++] = vcpupin; and that's why it's not "1". It is expected behavior and the problem lies in the check for the vcpupin where we should check nvcpupin instead.
but I'll check the other places where we format it as well.
As I said, I'll have a look where else it is used incorrectly. Will send a v2 for that.
And as long as we try to use nvcpupin here, we should use nvcpupin when formating "</cputune>" too.
Osier
We can use VIR_REALLOC_N with NULL pointer, which behaves the same way as VIR_ALLOC_N in that case, so no need for a condition that's checking if some data are allocated already. --- I tried to find other parts of the code similar to this, so I can do a full cleanup for the whole repository, so I used this (excuse the long line, but that's how I was writing it): git grep -nHC 5 -e VIR_REALLOC_N -e VIR_ALLOC_N | while read line; do if [[ "$line" == "--" ]]; then if [[ ${#tmpbuf} -gt 10 && "$REALLOC_N" == "true" && "$ALLOC_N" == "true" ]]; then echo $line; while [[ ${#tmpbuf[*]} -gt 0 ]]; do echo "${tmpbuf[0]}"; tmpbuf=( "${tmpbuf[@]:1:${#tmpbuf[*]}}" ); done; fi; unset tmpbuf REALLOC_N ALLOC_N; else if [[ "$ALLOC_N" != "true" && "${line/VIR_ALLOC_N//}" != "${line}" ]]; then ALLOC_N="true"; fi; if [[ "$REALLOC_N" != "true" && "${line/VIR_REALLOC_N//}" != "${line}" ]]; then REALLOC_N="true"; fi; tmpbuf[${#tmpbuf[*]}]="$line"; fi; done | less And reviewed the output just to find out this was the only occurrence of the inconsistency. --- src/conf/domain_conf.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 329ada3..21d67a3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9047,16 +9047,9 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, * the policy specified explicitly as def->cpuset. */ if (def->cpumask) { - if (!def->cputune.vcpupin) { - if (VIR_ALLOC_N(def->cputune.vcpupin, def->vcpus) < 0) { - virReportOOMError(); - goto error; - } - } else { - if (VIR_REALLOC_N(def->cputune.vcpupin, def->vcpus) < 0) { - virReportOOMError(); - goto error; - } + if (VIR_REALLOC_N(def->cputune.vcpupin, def->vcpus) < 0) { + virReportOOMError(); + goto error; } for (i = 0; i < def->vcpus; i++) { -- 1.8.0.2
On 2012年12月17日 23:17, Martin Kletzander wrote:
We can use VIR_REALLOC_N with NULL pointer, which behaves the same way as VIR_ALLOC_N in that case, so no need for a condition that's checking if some data are allocated already.
---
I tried to find other parts of the code similar to this, so I can do a full cleanup for the whole repository, so I used this (excuse the long line, but that's how I was writing it):
git grep -nHC 5 -e VIR_REALLOC_N -e VIR_ALLOC_N | while read line; do if [[ "$line" == "--" ]]; then if [[ ${#tmpbuf} -gt 10&& "$REALLOC_N" == "true"&& "$ALLOC_N" == "true" ]]; then echo $line; while [[ ${#tmpbuf[*]} -gt 0 ]]; do echo "${tmpbuf[0]}"; tmpbuf=( "${tmpbuf[@]:1:${#tmpbuf[*]}}" ); done; fi; unset tmpbuf REALLOC_N ALLOC_N; else if [[ "$ALLOC_N" != "true"&& "${line/VIR_ALLOC_N//}" != "${line}" ]]; then ALLOC_N="true"; fi; if [[ "$REALLOC_N" != "true"&& "${line/VIR_REALLOC_N//}" != "${line}" ]]; then REALLOC_N="true"; fi; tmpbuf[${#tmpbuf[*]}]="$line"; fi; done | less
And reviewed the output just to find out this was the only occurrence of the inconsistency. --- src/conf/domain_conf.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 329ada3..21d67a3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9047,16 +9047,9 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, * the policy specified explicitly as def->cpuset. */ if (def->cpumask) { - if (!def->cputune.vcpupin) { - if (VIR_ALLOC_N(def->cputune.vcpupin, def->vcpus)< 0) { - virReportOOMError(); - goto error; - } - } else { - if (VIR_REALLOC_N(def->cputune.vcpupin, def->vcpus)< 0) { - virReportOOMError(); - goto error; - } + if (VIR_REALLOC_N(def->cputune.vcpupin, def->vcpus)< 0) { + virReportOOMError(); + goto error; }
I think sometimes one will still want the VIR_ALLOC_N, for it fills the allocated memory with zeros, but not uninitialized values. it's not common though. Or may be we should use VIR_EXPAND_N in that case. But here we don't have to worry about the uninitialized values indeed. So ACK. Osier
Removing "exempli gratia" when there should be dots afterwards, also Uppercase letter should be present when starting a sentence. Sorry for the double line in the last hunk even though it's just adding a word, but it would require to have new sentence starting on a newline and that didn't seem very nice. --- docs/hacking.html.in | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 40acdbb..1363779 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -217,7 +217,7 @@ <p> The keywords <code>if</code>, <code>for</code>, <code>while</code>, and <code>switch</code> must have a single space following them - before the opening bracket. eg + before the opening bracket. Example: </p> <pre> if(foo) // Bad @@ -226,7 +226,7 @@ <p> Function implementations must <strong>not</strong> have any whitespace - between the function name and the opening bracket. eg + between the function name and the opening bracket. Example: </p> <pre> int foo (int wizz) // Bad @@ -235,7 +235,7 @@ <p> Function calls must <strong>not</strong> have any whitespace - between the function name and the opening bracket. eg + between the function name and the opening bracket. Example: </p> <pre> bar = foo (wizz); // Bad @@ -245,7 +245,7 @@ <p> Function typedefs must <strong>not</strong> have any whitespace between the closing bracket of the function name and opening - bracket of the arg list. eg + bracket of the arg list. Example: </p> <pre> typedef int (*foo) (int wizz); // Bad @@ -253,8 +253,8 @@ </pre> <p> - There must not be any whitespace immediately following any - opening bracket, or immediately prior to any closing bracket + There must not be any whitespace immediately following any opening + bracket, or immediately prior to any closing bracket. Example: </p> <pre> int foo( int wizz ); // Bad -- 1.8.0.2
On 2012年12月17日 23:17, Martin Kletzander wrote:
Removing "exempli gratia" when there should be dots afterwards, also Uppercase letter should be present when starting a sentence. Sorry for the double line in the last hunk even though it's just adding a word, but it would require to have new sentence starting on a newline and that didn't seem very nice. --- docs/hacking.html.in | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 40acdbb..1363779 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -217,7 +217,7 @@ <p> The keywords<code>if</code>,<code>for</code>,<code>while</code>, and<code>switch</code> must have a single space following them - before the opening bracket. eg + before the opening bracket. Example:
Not sure I like this, as I'm a fan of using "e.g.". :-) And isn't "e.g." is used everywhere across the project? Should we just fix it by s/eg/e.g./? Osier
On 12/18/2012 04:29 AM, Osier Yang wrote:
On 2012年12月17日 23:17, Martin Kletzander wrote:
Removing "exempli gratia" when there should be dots afterwards, also Uppercase letter should be present when starting a sentence. Sorry for the double line in the last hunk even though it's just adding a word, but it would require to have new sentence starting on a newline and that didn't seem very nice. --- docs/hacking.html.in | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 40acdbb..1363779 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -217,7 +217,7 @@ <p> The keywords<code>if</code>,<code>for</code>,<code>while</code>, and<code>switch</code> must have a single space following them - before the opening bracket. eg + before the opening bracket. Example:
Not sure I like this, as I'm a fan of using "e.g.". :-)
And isn't "e.g." is used everywhere across the project? Should we just fix it by s/eg/e.g./?
Osier
I've no problem with that and will gladly send a v2 for this. Thanks for the review, I've pushed the ACKed ones. Martin
participants (2)
-
Martin Kletzander -
Osier Yang