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@xxxxxxxxxx>
>> ---
>> 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;
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list