11:37 a.m.
On 04/21/2015 10:08 AM, Peter Krempa wrote:
On Tue, Apr 14, 2015 at 21:18:21 -0400, John Ferlan wrote:
> Adding a new XML element 'iothreadids' in order to allow defining
> specific IOThread ID's rather than relying on the algorithm to assign
> IOThread ID's starting at 1 and incrementing to iothreads count.
>
> This will allow future patches to be able to add new IOThreads by
> a specific iothread_id and of course delete any exisiting IOThread.
>
> Each iothreadids element will have 'n' <iothread> children elements
> which will have attribute "id". The "id" will allow for
definition
> of any "valid" (eg > 0) iothread_id value.
>
> On input, if any <iothreadids> <iothread>'s are provided, they will
> be marked so that we only print out what we read in.
>
> On input, if no <iothreadids> are provided, the PostParse code will
> self generate a list of ID's starting at 1 and going to the number
> of iothreads defined for the domain (just like the current algorithm
> numbering scheme). A future patch will rework the existing algorithm
> to make use of the iothreadids list.
>
> On output, only print out the <iothreadids> if they were read in.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> docs/formatdomain.html.in | 30 +++++++
> docs/schemas/domaincommon.rng | 12 +++
> src/conf/domain_conf.c | 190 +++++++++++++++++++++++++++++++++++++++++-
> src/conf/domain_conf.h | 15 ++++
> src/libvirt_private.syms | 3 +
> 5 files changed, 248 insertions(+), 2 deletions(-)
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index e921749..518f7c5 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -521,6 +521,18 @@
> ...
> </domain>
> </pre>
> +<pre>
> +<domain>
> + ...
> + <iothreadids>
> + <iothread id="2"/>
> + <iothread id="4"/>
> + <iothread id="6"/>
> + <iothread id="8"/>
> + </iothreadids>
> + ...
> +</domain>
> +</pre>
>
> <dl>
> <dt><code>iothreads</code></dt>
> @@ -530,7 +542,25 @@
> virtio-blk-pci and virtio-blk-ccw target storage devices. There
> should be only 1 or 2 IOThreads per host CPU. There may be more
> than one supported device assigned to each IOThread.
> + <span class="since">Since 1.2.8</span>
> </dd>
> + <dt><code>iothreadids</code></dt>
> + <dd>
> + The optional <code>iothreadids</code> element provides the
capability
> + to specifically define the IOThread ID's for the domain. By default,
> + IOThread ID's are sequentially numbered starting from 1 through the
> + number of <code>iothreads</code> defined for the domain. The
> + <code>id</code> attribute is used to define the IOThread ID.
The
> + <code>id</code> attribute must be a positive integer greater
than 0.
> + If there are less <code>iothreadids</code> defined than
> + <code>iothreads</code> defined for the domain, then libvirt
will
> + sequentially fill <code>iothreadids</code> starting at 1
avoiding
> + any predefined <code>id</code>. If there are more
> + <code>iothreadids</code> defined than
<code>iothreads</code>
> + defined for the domain, then the <code>iothreads</code> value
> + will be adjusted accordingly.
> + <span class="since">Since 1.2.15</span>
> + </dd>
> </dl>
>
> <h3><a name="elementsCPUTuning">CPU
Tuning</a></h3>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 03fd541..4bd32bd 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -675,6 +675,18 @@
> </optional>
>
> <optional>
> + <element name="iothreadids">
> + <zeroOrMore>
> + <element name="iothread">
> + <attribute name="id">
> + <ref name="unsignedInt"/>
> + </attribute>
> + </element>
> + </zeroOrMore>
> + </element>
> + </optional>
> +
> + <optional>
> <ref name="blkiotune"/>
> </optional>
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 4d7e3c9..e86b7e2 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2113,6 +2113,23 @@ virDomainPinDefCopy(virDomainPinDefPtr *src, int npin)
> return NULL;
> }
>
> +
> +static void
> +virDomainIOThreadIDDefArrayFree(virDomainIOThreadIDDefPtr *def,
> + int nids)
> +{
> + size_t i;
> +
> + if (!def)
> + return;
> +
> + for (i = 0; i < nids; i++)
> + VIR_FREE(def[i]);
> +
> + VIR_FREE(def);
> +}
> +
> +
> void
> virDomainPinDefFree(virDomainPinDefPtr def)
> {
> @@ -2310,6 +2327,8 @@ void virDomainDefFree(virDomainDefPtr def)
>
> virCPUDefFree(def->cpu);
>
> + virDomainIOThreadIDDefArrayFree(def->iothreadids, def->niothreadids);
> +
> virDomainPinDefArrayFree(def->cputune.vcpupin, def->cputune.nvcpupin);
>
> virDomainPinDefFree(def->cputune.emulatorpin);
> @@ -3304,6 +3323,18 @@ virDomainDefPostParseInternal(virDomainDefPtr def,
> return -1;
> }
>
> + /* Fully populate the IOThread ID list */
> + if (def->iothreads && def->iothreads != def->niothreadids) {
> + unsigned int iothread_id = 1;
> + while (def->niothreadids != def->iothreads) {
> + if (!virDomainIOThreadIDFind(def, iothread_id)) {
> + if (virDomainIOThreadIDAdd(def, iothread_id) < 0)
This code is _adding_ fields for every iothread that was not specified
in <iothreadids> but is implied by <iothreads>, but before that you've
allocated all the structures in [1].
Hmm... correct - of course the allocation in [1] was suggested by
previous code review.
> + return -1;
> + }
> + iothread_id++;
> + }
> + }
> +
> if (virDomainDefGetMemoryInitial(def) == 0) {
> virReportError(VIR_ERR_XML_ERROR, "%s",
> _("Memory size must be specified via <memory> or
in the "
> @@ -13173,6 +13204,56 @@ virDomainIdmapDefParseXML(xmlXPathContextPtr ctxt,
> return idmap;
> }
>
> +/* Parse the XML definition for an IOThread ID
> + *
> + * Format is :
> + *
> + * <iothreads>4</iothreads>
> + * <iothreadids>
> + * <iothread id='1'/>
> + * <iothread id='3'/>
> + * <iothread id='5'/>
> + * <iothread id='7'/>
> + * </iothreadids>
> + */
> +static virDomainIOThreadIDDefPtr
> +virDomainIOThreadIDDefParseXML(xmlNodePtr node,
> + xmlXPathContextPtr ctxt)
> +{
> + virDomainIOThreadIDDefPtr iothrid;
> + xmlNodePtr oldnode = ctxt->node;
> + char *tmp = NULL;
> +
> + if (VIR_ALLOC(iothrid) < 0)
> + return NULL;
> +
> + ctxt->node = node;
> +
> + if (!(tmp = virXPathString("string(./@id)", ctxt))) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("Missing 'id' attribute in <iothread>
element"));
> + goto error;
> + }
> + if (virStrToLong_uip(tmp, NULL, 10, &iothrid->iothread_id) < 0 ||
> + iothrid->iothread_id == 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("invalid iothread 'id' value
'%s'"), tmp);
> + goto error;
> + }
> +
> + iothrid->defined = true;
> +
> + cleanup:
> + VIR_FREE(tmp);
> + ctxt->node = oldnode;
> + return iothrid;
> +
> + error:
> + VIR_FREE(iothrid);
> + goto cleanup;
> +}
> +
> +
> /* Parse the XML definition for a vcpupin
> *
> * vcpupin has the form of
> @@ -13948,6 +14029,32 @@ virDomainDefParseXML(xmlDocPtr xml,
> }
> VIR_FREE(tmp);
>
> + /* Extract any iothread id's defined */
> + if ((n = virXPathNodeSet("./iothreadids/iothread", ctxt, &nodes))
< 0)
> + goto error;
> +
> + if (n > def->iothreads)
> + def->iothreads = n;
> +
> + if (n && VIR_ALLOC_N(def->iothreadids, MAX(n, def->iothreads))
< 0)
> + goto error;
[1]
Yes, the MAX(n, def->iothreads) was the prior code review suggestion...
So if I 'revert' back to just 'n', then let the above code handle the
rest, then these two issues should be resolved.
> +
> + for (i = 0; i < n; i++) {
> + virDomainIOThreadIDDefPtr iothrid = NULL;
> + if (!(iothrid = virDomainIOThreadIDDefParseXML(nodes[i], ctxt)))
> + goto error;
> +
> + if (virDomainIOThreadIDFind(def, iothrid->iothread_id)) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("duplicate iothread id '%u' found"),
> + iothrid->iothread_id);
> + VIR_FREE(iothrid);
> + goto error;
> + }
> + def->iothreadids[def->niothreadids++] = iothrid;
> + }
> + VIR_FREE(nodes);
> +
> /* Extract cpu tunables. */
> if ((n = virXPathULong("string(./cputune/shares[1])", ctxt,
> &def->cputune.shares)) < -1) {
> @@ -17324,6 +17431,66 @@ virDomainDefAddImplicitControllers(virDomainDefPtr def)
> return 0;
> }
>
> +virDomainIOThreadIDDefPtr
> +virDomainIOThreadIDFind(virDomainDefPtr def,
> + unsigned int iothread_id)
> +{
> + size_t i;
> +
> + if (!def->iothreadids || !def->niothreadids)
> + return NULL;
> +
> + for (i = 0; i < def->niothreadids; i++) {
> + if (iothread_id == def->iothreadids[i]->iothread_id)
> + return def->iothreadids[i];
> + }
> +
> + return NULL;
> +}
> +
> +int
> +virDomainIOThreadIDAdd(virDomainDefPtr def,
> + unsigned int iothread_id)
If this function returned the pointer to the added structure you'd be
able to use it later to modify other aspects of it.
OK seems reasonable... had to change to using VIR_APPEND_ELEMENT_COPY too
> +{
> + virDomainIOThreadIDDefPtr iothrid = NULL;
> +
> + if (virDomainIOThreadIDFind(def, iothread_id)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("cannot duplicate iothread_id '%u' in
iothreadids"),
> + iothread_id);
> + return -1;
> + }
> +
> + if (VIR_ALLOC(iothrid) < 0)
> + goto error;
> +
> + iothrid->iothread_id = iothread_id;
> +
> + if (VIR_APPEND_ELEMENT(def->iothreadids, def->niothreadids, iothrid) <
0)
> + goto error;
> +
> + return 0;
> +
> + error:
> + VIR_FREE(iothrid);
> + return -1;
> +}
> +
> +void
> +virDomainIOThreadIDDel(virDomainDefPtr def,
> + unsigned int iothread_id)
> +{
> + int n;
> +
> + for (n = 0; n < def->niothreadids; n++) {
> + if (def->iothreadids[n]->iothread_id == iothread_id) {
> + VIR_FREE(def->iothreadids[n]);
This could use a common freeing function, so that once you add more data
you won't have to tweak it.
So you'd prefer a 'common' free function that just calls VIR_FREE() -
that just seems like excessive overhead, but that's fine.
> + VIR_DELETE_ELEMENT(def->iothreadids, n,
def->niothreadids);
> + return;
> + }
> + }
> +}
> +
> /* Check if vcpupin with same id already exists. */
> bool
> virDomainPinIsDuplicate(virDomainPinDefPtr *def,
> @@ -20666,8 +20833,27 @@ virDomainDefFormatInternal(virDomainDefPtr def,
> virBufferAsprintf(buf, " current='%u'", def->vcpus);
> virBufferAsprintf(buf, ">%u</vcpu>\n", def->maxvcpus);
>
> - if (def->iothreads > 0)
> - virBufferAsprintf(buf, "<iothreads>%u</iothreads>\n",
def->iothreads);
> + if (def->iothreads > 0) {
> + virBufferAsprintf(buf, "<iothreads>%u</iothreads>\n",
> + def->iothreads);
> + /* If we parsed the iothreadids, then write out those that we parsed */
Seems rather obvious.
As a reviewer for now, sure... But what about someone who reads this
code 6, 12, 18 months from now and wonders why the extra loop...
I can adjust the comment to be :
"Only print out iothreadids if we read at least one"
> + for (i = 0; i < def->niothreadids; i++) {
> + if (def->iothreadids[i]->defined)
> + break;
> + }
> + if (i < def->niothreadids) {
> + virBufferAddLit(buf, "<iothreadids>\n");
> + virBufferAdjustIndent(buf, 2);
> + for (i = 0; i < def->niothreadids; i++) {
> + if (!def->iothreadids[i]->defined)
> + continue;
> + virBufferAsprintf(buf, "<iothread
id='%u'/>\n",
> + def->iothreadids[i]->iothread_id);
> + }
> + virBufferAdjustIndent(buf, -2);
> + virBufferAddLit(buf, "</iothreadids>\n");
> + }
> + }
>
> if (def->cputune.sharesSpecified ||
> (def->cputune.nvcpupin && !virDomainIsAllVcpupinInherited(def))
||
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index e6fa3c9..9e7bdf9 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2041,6 +2041,14 @@ struct _virDomainHugePage {
> unsigned long long size; /* hugepage size in KiB */
> };
>
> +typedef struct _virDomainIOThreadIDDef virDomainIOThreadIDDef;
> +typedef virDomainIOThreadIDDef *virDomainIOThreadIDDefPtr;
> +
> +struct _virDomainIOThreadIDDef {
> + bool defined;
You may want to invert this field, since the case where you add the
thread structs due to the fact that <iohreads> was more than the count
of entries is less common than other places that add the struct.
Made for an awkward looking:
for (i = 0; i < def->niothreadids; i++) {
if (!def->iothreadids[i]->undefined)
break;
}
> + unsigned int iothread_id;
> +};
> +
> typedef struct _virDomainCputune virDomainCputune;
> typedef virDomainCputune *virDomainCputunePtr;
>
> @@ -2132,6 +2140,8 @@ struct _virDomainDef {
> virBitmapPtr cpumask;
>
> unsigned int iothreads;
> + size_t niothreadids;
> + virDomainIOThreadIDDefPtr *iothreadids;
>
> virDomainCputune cputune;
>
> @@ -2590,6 +2600,11 @@ bool virDomainDefCheckABIStability(virDomainDefPtr src,
>
> int virDomainDefAddImplicitControllers(virDomainDefPtr def);
>
> +virDomainIOThreadIDDefPtr
> +virDomainIOThreadIDFind(virDomainDefPtr def, unsigned int iothread_id);
Usually the return type is on the same line and the argument list is
broken if it doesn't fit.
OK - perhaps a relic of previous code where even the first argument made
the line too long - been too long to remember for sure.
> +int virDomainIOThreadIDAdd(virDomainDefPtr def, unsigned int
iothread_id);
> +void virDomainIOThreadIDDel(virDomainDefPtr def, unsigned int iothread_id);
> +
> unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags);
>
> char *virDomainDefFormat(virDomainDefPtr def,
Which gives the following to be squashed in (trying to make progress on
at least the first two patches which compile and pass tests):
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 82c36a4..3ff06b6 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2115,6 +2115,14 @@ virDomainPinDefCopy(virDomainPinDefPtr *src, int
npin)
}
+void
+virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def)
+{
+ if (def)
+ VIR_FREE(def);
+}
+
+
static void
virDomainIOThreadIDDefArrayFree(virDomainIOThreadIDDefPtr *def,
int nids)
@@ -2125,7 +2133,7 @@
virDomainIOThreadIDDefArrayFree(virDomainIOThreadIDDefPtr *def,
return;
for (i = 0; i < nids; i++)
- VIR_FREE(def[i]);
+ virDomainIOThreadIDDefFree(def[i]);
VIR_FREE(def);
}
@@ -3322,8 +3330,11 @@ virDomainDefPostParseInternal(virDomainDefPtr def,
unsigned int iothread_id = 1;
while (def->niothreadids != def->iothreads) {
if (!virDomainIOThreadIDFind(def, iothread_id)) {
- if (virDomainIOThreadIDAdd(def, iothread_id) < 0)
+ virDomainIOThreadIDDefPtr iothrid;
+
+ if (!(iothrid = virDomainIOThreadIDAdd(def, iothread_id)))
return -1;
+ iothrid->undefined = true;
}
iothread_id++;
}
@@ -13216,15 +13227,13 @@ virDomainIOThreadIDDefParseXML(xmlNodePtr node,
goto error;
}
- iothrid->defined = true;
-
cleanup:
VIR_FREE(tmp);
ctxt->node = oldnode;
return iothrid;
error:
- VIR_FREE(iothrid);
+ virDomainIOThreadIDDefFree(iothrid);
goto cleanup;
}
@@ -14042,7 +14051,7 @@ virDomainDefParseXML(xmlDocPtr xml,
if (n > def->iothreads)
def->iothreads = n;
- if (n && VIR_ALLOC_N(def->iothreadids, MAX(n, def->iothreads)) < 0)
+ if (n && VIR_ALLOC_N(def->iothreadids, n) < 0)
goto error;
for (i = 0; i < n; i++) {
@@ -14054,7 +14063,7 @@ virDomainDefParseXML(xmlDocPtr xml,
virReportError(VIR_ERR_XML_ERROR,
_("duplicate iothread id '%u' found"),
iothrid->iothread_id);
- VIR_FREE(iothrid);
+ virDomainIOThreadIDDefFree(iothrid);
goto error;
}
def->iothreadids[def->niothreadids++] = iothrid;
@@ -17362,7 +17371,7 @@ virDomainIOThreadIDFind(virDomainDefPtr def,
return NULL;
}
-int
+virDomainIOThreadIDDefPtr
virDomainIOThreadIDAdd(virDomainDefPtr def,
unsigned int iothread_id)
{
@@ -17372,7 +17381,7 @@ virDomainIOThreadIDAdd(virDomainDefPtr def,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("cannot duplicate iothread_id '%u' in
iothreadids"),
iothread_id);
- return -1;
+ return NULL;
}
if (VIR_ALLOC(iothrid) < 0)
@@ -17380,14 +17389,15 @@ virDomainIOThreadIDAdd(virDomainDefPtr def,
iothrid->iothread_id = iothread_id;
- if (VIR_APPEND_ELEMENT(def->iothreadids, def->niothreadids,
iothrid) < 0)
+ if (VIR_APPEND_ELEMENT_COPY(def->iothreadids, def->niothreadids,
+ iothrid) < 0)
goto error;
- return 0;
+ return iothrid;
error:
- VIR_FREE(iothrid);
- return -1;
+ virDomainIOThreadIDDefFree(iothrid);
+ return NULL;
}
void
@@ -17398,7 +17408,7 @@ virDomainIOThreadIDDel(virDomainDefPtr def,
for (n = 0; n < def->niothreadids; n++) {
if (def->iothreadids[n]->iothread_id == iothread_id) {
- VIR_FREE(def->iothreadids[n]);
+ virDomainIOThreadIDDefFree(def->iothreadids[n]);
VIR_DELETE_ELEMENT(def->iothreadids, n, def->niothreadids);
return;
}
@@ -20749,16 +20759,16 @@ virDomainDefFormatInternal(virDomainDefPtr def,
if (def->iothreads > 0) {
virBufferAsprintf(buf, "<iothreads>%u</iothreads>\n",
def->iothreads);
- /* If we parsed the iothreadids, then write out those that we
parsed */
+ /* Only print out iothreadids if we read at least one */
for (i = 0; i < def->niothreadids; i++) {
- if (def->iothreadids[i]->defined)
+ if (!def->iothreadids[i]->undefined)
break;
}
if (i < def->niothreadids) {
virBufferAddLit(buf, "<iothreadids>\n");
virBufferAdjustIndent(buf, 2);
for (i = 0; i < def->niothreadids; i++) {
- if (!def->iothreadids[i]->defined)
+ if (def->iothreadids[i]->undefined)
continue;
virBufferAsprintf(buf, "<iothread id='%u'/>\n",
def->iothreadids[i]->iothread_id);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 01dc1f9..32de301 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2058,10 +2058,12 @@ typedef struct _virDomainIOThreadIDDef
virDomainIOThreadIDDef;
typedef virDomainIOThreadIDDef *virDomainIOThreadIDDefPtr;
struct _virDomainIOThreadIDDef {
- bool defined;
+ bool undefined;
unsigned int iothread_id;
};
+void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def);
+
typedef struct _virDomainCputune virDomainCputune;
typedef virDomainCputune *virDomainCputunePtr;
@@ -2612,9 +2614,10 @@ bool
virDomainDefCheckABIStability(virDomainDefPtr src,
int virDomainDefAddImplicitControllers(virDomainDefPtr def);
-virDomainIOThreadIDDefPtr
-virDomainIOThreadIDFind(virDomainDefPtr def, unsigned int iothread_id);
-int virDomainIOThreadIDAdd(virDomainDefPtr def, unsigned int iothread_id);
+virDomainIOThreadIDDefPtr virDomainIOThreadIDFind(virDomainDefPtr def,
+ unsigned int
iothread_id);
+virDomainIOThreadIDDefPtr virDomainIOThreadIDAdd(virDomainDefPtr def,
+ unsigned int iothread_id);
void virDomainIOThreadIDDel(virDomainDefPtr def, unsigned int iothread_id);
unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 649ed8f..ac35f1b 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -325,6 +325,7 @@ virDomainHypervTypeFromString;
virDomainHypervTypeToString;
virDomainInputDefFree;
virDomainIOThreadIDAdd;
+virDomainIOThreadIDDefFree;
virDomainIOThreadIDDel;
virDomainIOThreadIDFind;
virDomainLeaseDefFree;