8:27 a.m.
This patch series introduces the support for new s390x 'loadparm'
feature. The 'loadparm' can be used to select the boot entry to
boot from, for a boot device.
Here is a link to the QEMU patches:
https://lists.nongnu.org/archive/html/qemu-devel/2017-05/msg00192.html
ChangeLog
---------
v2 -> v3:
- Updated news.xml and formatdomain.html.in with a more architectural
description of loadparm (patch 1)
v1 -> v2:
- Rebased the patch series on the latest master, commit
2f69dd3 virfiletest: include linux/falloc.h
Thanks
Farhan Ali
Farhan Ali (3):
conf : Add loadparm boot option for a boot device
qemu : Add loadparm to qemu command line string
tests : Testcases for loadparm
docs/formatdomain.html.in | 9 ++-
docs/news.xml | 11 ++++
docs/schemas/domaincommon.rng | 7 +++
src/conf/device_conf.h | 1 +
src/conf/domain_conf.c | 69 +++++++++++++++++++++-
src/qemu/qemu_capabilities.c | 2 +
src/qemu/qemu_capabilities.h | 1 +
src/qemu/qemu_command.c | 37 ++++++++++++
...-machine-loadparm-multiple-disks-nets-s390.args | 28 +++++++++
...v-machine-loadparm-multiple-disks-nets-s390.xml | 43 ++++++++++++++
.../qemuxml2argv-machine-loadparm-net-s390.args | 20 +++++++
.../qemuxml2argv-machine-loadparm-net-s390.xml | 26 ++++++++
...xml2argv-machine-loadparm-s390-char-invalid.xml | 26 ++++++++
...uxml2argv-machine-loadparm-s390-len-invalid.xml | 26 ++++++++
.../qemuxml2argv-machine-loadparm-s390.args | 20 +++++++
.../qemuxml2argv-machine-loadparm-s390.xml | 26 ++++++++
tests/qemuxml2argvtest.c | 19 ++++++
17 files changed, 367 insertions(+), 4 deletions(-)
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-multiple-disks-nets-s390.args
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-multiple-disks-nets-s390.xml
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-net-s390.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-net-s390.xml
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-s390-char-invalid.xml
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-s390-len-invalid.xml
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-s390.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-s390.xml
--
1.9.1
8:27 a.m.
New subject: [libvirt] [PATCH v3 1/3] conf : Add loadparm boot option for a boot device
Update the per device boot schema to add an optional loadparm parameter.
Extend the virDomainDeviceInfo to support loadparm option.
Modify the appropriate functions to parse loadparm from boot device xml.
Signed-off-by: Farhan Ali <alifm(a)linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
Reviewed-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
---
docs/formatdomain.html.in | 9 ++++--
docs/news.xml | 11 +++++++
docs/schemas/domaincommon.rng | 7 +++++
src/conf/device_conf.h | 1 +
src/conf/domain_conf.c | 69 +++++++++++++++++++++++++++++++++++++++++--
5 files changed, 93 insertions(+), 4 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 3135db4..fd6f666 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -3022,8 +3022,13 @@
<dt><code>boot</code></dt>
<dd>Specifies that the disk is bootable. The <code>order</code>
attribute determines the order in which devices will be tried during
- boot sequence. The per-device <code>boot</code> elements cannot be
- used together with general boot elements in
+ boot sequence. On S390 architecture only the first boot device is used.
+ The optional <code>loadparm</code> attribute is an 8 byte parameter
+ which can be queried by guests on S390 via sclp or diag 308. Linux
+ guests on S390 can use <code>loadparm</code> to select a boot entry.
+ <span class="since">Since 3.4.0</span>
+ The per-device <code>boot</code> elements cannot be used together
+ with general boot elements in
<a href="#elementsOSBIOS">BIOS bootloader</a> section.
<span class="since">Since 0.8.8</span>
</dd>
diff --git a/docs/news.xml b/docs/news.xml
index 52915ee..bd9e43a 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -37,6 +37,17 @@
<section title="New features">
<change>
<summary>
+ qemu: Add support for loadparm for a boot device
+ </summary>
+ <description>
+ Add an optional boot parameter 'loadparm' for a boot device.
+ Loadparm is a 8 byte parameter than can be queried by S390 guests
+ via sclp or diag 308. Linux guests on S390 use it to select a boot
+ entry.
+ </description>
+ </change>
+ <change>
+ <summary>
Improved streams to efficiently transfer sparseness
</summary>
<description>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index f88e84a..c885aec 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -5026,6 +5026,13 @@
<attribute name="order">
<ref name="positiveInteger"/>
</attribute>
+ <optional>
+ <attribute name="loadparm">
+ <data type="string">
+ <param name="pattern">[a-zA-Z0-9.\s]{1,8}</param>
+ </data>
+ </attribute>
+ </optional>
<empty/>
</element>
</define>
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
index a20de85..48782be 100644
--- a/src/conf/device_conf.h
+++ b/src/conf/device_conf.h
@@ -167,6 +167,7 @@ struct _virDomainDeviceInfo {
* assignment, never saved and never reported.
*/
int pciConnectFlags; /* enum virDomainPCIConnectFlags */
+ char *loadparm;
};
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 8c62673..dbf6108 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3520,6 +3520,7 @@ void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
memset(&info->addr, 0, sizeof(info->addr));
info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
VIR_FREE(info->romfile);
+ VIR_FREE(info->loadparm);
}
@@ -5199,6 +5200,48 @@ virDomainDefValidate(virDomainDefPtr def,
return 0;
}
+/**
+ * virDomainDeviceLoadparmIsValid
+ * @loadparm : The string to validate
+ *
+ * The valid set of values for loadparm are [a-zA-Z0-9.]
+ * and blank spaces.
+ * The maximum allowed length is 8 characters.
+ * An empty string is considered invalid
+ */
+static bool
+virDomainDeviceLoadparmIsValid(const char *loadparm)
+{
+ size_t i;
+
+ if (virStringIsEmpty(loadparm)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("loadparm cannot be an empty string"));
+ return false;
+ }
+
+ if (strlen(loadparm) > 8) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("loadparm '%s' exceeds 8 characters"),
loadparm);
+ return false;
+ }
+
+ for (i = 0; i < strlen(loadparm); i++) {
+ uint8_t c = loadparm[i];
+
+ if (('A' <= c && c <= 'Z') || ('0' <= c
&& c <= '9') ||
+ (c == '.') || (c == ' ')) {
+ continue;
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("invalid loadparm char '%c', expecting
chars"
+ " in set of [a-zA-Z0-9.] and blank spaces"), c);
+ return false;
+ }
+ }
+
+ return true;
+}
/* Generate a string representation of a device address
* @info address Device address to stringify
@@ -5208,9 +5251,14 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
virDomainDeviceInfoPtr info,
unsigned int flags)
{
- if ((flags & VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) && info->bootIndex)
- virBufferAsprintf(buf, "<boot order='%u'/>\n",
info->bootIndex);
+ if ((flags & VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) && info->bootIndex) {
+ virBufferAsprintf(buf, "<boot order='%u'",
info->bootIndex);
+ if (info->loadparm)
+ virBufferAsprintf(buf, " loadparm='%s'",
info->loadparm);
+
+ virBufferAddLit(buf, "/>\n");
+ }
if (info->alias &&
!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) {
virBufferAsprintf(buf, "<alias name='%s'/>\n",
info->alias);
@@ -5636,6 +5684,7 @@ virDomainDeviceBootParseXML(xmlNodePtr node,
virHashTablePtr bootHash)
{
char *order;
+ char *loadparm;
int ret = -1;
if (!(order = virXMLPropString(node, "order"))) {
@@ -5664,10 +5713,26 @@ virDomainDeviceBootParseXML(xmlNodePtr node,
goto cleanup;
}
+ loadparm = virXMLPropString(node, "loadparm");
+ if (loadparm) {
+ if (virStringToUpper(&info->loadparm, loadparm) != 1) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to convert loadparm '%s' to upper
case"),
+ loadparm);
+ goto cleanup;
+ }
+
+ if (!virDomainDeviceLoadparmIsValid(info->loadparm)) {
+ VIR_FREE(info->loadparm);
+ goto cleanup;
+ }
+ }
+
ret = 0;
cleanup:
VIR_FREE(order);
+ VIR_FREE(loadparm);
return ret;
}
--
1.9.1
6:58 p.m.
New subject: [libvirt] [PATCH v3 1/3] conf : Add loadparm boot option for a boot device
On 05/23/2017 09:27 AM, Farhan Ali wrote:
Update the per device boot schema to add an optional loadparm
parameter.
Extend the virDomainDeviceInfo to support loadparm option.
Modify the appropriate functions to parse loadparm from boot device xml.
FWIW: I got confused by the time I got to patch 3... the "loadparm" add
added to the -machine qemu command line, but the libvirt XML is added to
the disk device entry.
Perhaps just add a sample here to show where the XML gets added.
Signed-off-by: Farhan Ali <alifm(a)linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
Reviewed-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
---
docs/formatdomain.html.in | 9 ++++--
docs/news.xml | 11 +++++++
Patch ordering - the docs/news.xml should be a single commit and would
be the the last commit. Still at least you have it there.
docs/schemas/domaincommon.rng | 7 +++++
src/conf/device_conf.h | 1 +
src/conf/domain_conf.c | 69 +++++++++++++++++++++++++++++++++++++++++--
When you change/add RNG/XML - there should be adjustments to
qemuxml2xmltest.c and of course xml2xml tests. Many examples of this to
draw from.
5 files changed, 93 insertions(+), 4 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 3135db4..fd6f666 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -3022,8 +3022,13 @@
<dt><code>boot</code></dt>
<dd>Specifies that the disk is bootable. The <code>order</code>
attribute determines the order in which devices will be tried during
- boot sequence. The per-device <code>boot</code> elements cannot be
- used together with general boot elements in
+ boot sequence. On S390 architecture only the first boot device is used.
+ The optional <code>loadparm</code> attribute is an 8 byte parameter
+ which can be queried by guests on S390 via sclp or diag 308. Linux
+ guests on S390 can use <code>loadparm</code> to select a boot
entry.
+ <span class="since">Since 3.4.0</span>
Since DV is cutting 3.4.0 tomorrow, this probably slides into 3.5.0.
Given when I realized by patch 2, probably no big deal!
+ The per-device <code>boot</code> elements cannot
be used together
+ with general boot elements in
<a href="#elementsOSBIOS">BIOS bootloader</a> section.
<span class="since">Since 0.8.8</span>
</dd>
diff --git a/docs/news.xml b/docs/news.xml
index 52915ee..bd9e43a 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -37,6 +37,17 @@
<section title="New features">
<change>
<summary>
+ qemu: Add support for loadparm for a boot device
+ </summary>
+ <description>
+ Add an optional boot parameter 'loadparm' for a boot device.
+ Loadparm is a 8 byte parameter than can be queried by S390 guests
s/a 8/an 8/
s/can be/when present is/ ?
+ via sclp or diag 308. Linux guests on S390 use it to
select a boot
+ entry.
+ </description>
+ </change>
+ <change>
+ <summary>
Improved streams to efficiently transfer sparseness
</summary>
<description>
The rest seems fine -
John
diff --git a/docs/schemas/domaincommon.rng
b/docs/schemas/domaincommon.rng
index f88e84a..c885aec 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -5026,6 +5026,13 @@
<attribute name="order">
<ref name="positiveInteger"/>
</attribute>
+ <optional>
+ <attribute name="loadparm">
+ <data type="string">
+ <param name="pattern">[a-zA-Z0-9.\s]{1,8}</param>
+ </data>
+ </attribute>
+ </optional>
<empty/>
</element>
</define>
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
index a20de85..48782be 100644
--- a/src/conf/device_conf.h
+++ b/src/conf/device_conf.h
@@ -167,6 +167,7 @@ struct _virDomainDeviceInfo {
* assignment, never saved and never reported.
*/
int pciConnectFlags; /* enum virDomainPCIConnectFlags */
+ char *loadparm;
};
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 8c62673..dbf6108 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3520,6 +3520,7 @@ void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
memset(&info->addr, 0, sizeof(info->addr));
info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
VIR_FREE(info->romfile);
+ VIR_FREE(info->loadparm);
}
@@ -5199,6 +5200,48 @@ virDomainDefValidate(virDomainDefPtr def,
return 0;
}
+/**
+ * virDomainDeviceLoadparmIsValid
+ * @loadparm : The string to validate
+ *
+ * The valid set of values for loadparm are [a-zA-Z0-9.]
+ * and blank spaces.
+ * The maximum allowed length is 8 characters.
+ * An empty string is considered invalid
+ */
+static bool
+virDomainDeviceLoadparmIsValid(const char *loadparm)
+{
+ size_t i;
+
+ if (virStringIsEmpty(loadparm)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("loadparm cannot be an empty string"));
+ return false;
+ }
+
+ if (strlen(loadparm) > 8) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("loadparm '%s' exceeds 8 characters"),
loadparm);
+ return false;
+ }
+
+ for (i = 0; i < strlen(loadparm); i++) {
+ uint8_t c = loadparm[i];
+
+ if (('A' <= c && c <= 'Z') || ('0' <= c
&& c <= '9') ||
+ (c == '.') || (c == ' ')) {
+ continue;
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("invalid loadparm char '%c', expecting
chars"
+ " in set of [a-zA-Z0-9.] and blank spaces"), c);
+ return false;
+ }
+ }
+
+ return true;
+}
/* Generate a string representation of a device address
* @info address Device address to stringify
@@ -5208,9 +5251,14 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
virDomainDeviceInfoPtr info,
unsigned int flags)
{
- if ((flags & VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) && info->bootIndex)
- virBufferAsprintf(buf, "<boot order='%u'/>\n",
info->bootIndex);
+ if ((flags & VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) && info->bootIndex) {
+ virBufferAsprintf(buf, "<boot order='%u'",
info->bootIndex);
+ if (info->loadparm)
+ virBufferAsprintf(buf, " loadparm='%s'",
info->loadparm);
+
+ virBufferAddLit(buf, "/>\n");
+ }
if (info->alias &&
!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) {
virBufferAsprintf(buf, "<alias name='%s'/>\n",
info->alias);
@@ -5636,6 +5684,7 @@ virDomainDeviceBootParseXML(xmlNodePtr node,
virHashTablePtr bootHash)
{
char *order;
+ char *loadparm;
int ret = -1;
if (!(order = virXMLPropString(node, "order"))) {
@@ -5664,10 +5713,26 @@ virDomainDeviceBootParseXML(xmlNodePtr node,
goto cleanup;
}
+ loadparm = virXMLPropString(node, "loadparm");
+ if (loadparm) {
+ if (virStringToUpper(&info->loadparm, loadparm) != 1) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to convert loadparm '%s' to upper
case"),
+ loadparm);
+ goto cleanup;
+ }
+
+ if (!virDomainDeviceLoadparmIsValid(info->loadparm)) {
+ VIR_FREE(info->loadparm);
+ goto cleanup;
+ }
+ }
+
ret = 0;
cleanup:
VIR_FREE(order);
+ VIR_FREE(loadparm);
return ret;
}
9:19 a.m.
New subject: [libvirt] [PATCH v3 1/3] conf : Add loadparm boot option for a boot device
On 05/25/2017 07:58 PM, John Ferlan wrote:
On 05/23/2017 09:27 AM, Farhan Ali wrote:
> Update the per device boot schema to add an optional loadparm parameter.
> Extend the virDomainDeviceInfo to support loadparm option.
> Modify the appropriate functions to parse loadparm from boot device xml.
>
FWIW: I got confused by the time I got to patch 3... the "loadparm" add
added to the -machine qemu command line, but the libvirt XML is added to
the disk device entry.
Perhaps just add a sample here to show where the XML gets added.
Sure, will add an example.
> Signed-off-by: Farhan Ali <alifm(a)linux.vnet.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
> Reviewed-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
> ---
> docs/formatdomain.html.in | 9 ++++--
> docs/news.xml | 11 +++++++
Patch ordering - the docs/news.xml should be a single commit and would
be the the last commit. Still at least you have it there.
Will move it as a separate commit.
> docs/schemas/domaincommon.rng | 7 +++++
> src/conf/device_conf.h | 1 +
> src/conf/domain_conf.c | 69 +++++++++++++++++++++++++++++++++++++++++--
When you change/add RNG/XML - there should be adjustments to
qemuxml2xmltest.c and of course xml2xml tests. Many examples of this to
draw from.
Sure, will add a test case for xml2xml tests.
> 5 files changed, 93 insertions(+), 4 deletions(-)
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 3135db4..fd6f666 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -3022,8 +3022,13 @@
> <dt><code>boot</code></dt>
> <dd>Specifies that the disk is bootable. The
<code>order</code>
> attribute determines the order in which devices will be tried during
> - boot sequence. The per-device <code>boot</code> elements cannot
be
> - used together with general boot elements in
> + boot sequence. On S390 architecture only the first boot device is used.
> + The optional <code>loadparm</code> attribute is an 8 byte
parameter
> + which can be queried by guests on S390 via sclp or diag 308. Linux
> + guests on S390 can use <code>loadparm</code> to select a boot
entry.
> + <span class="since">Since 3.4.0</span>
Since DV is cutting 3.4.0 tomorrow, this probably slides into 3.5.0.
Given when I realized by patch 2, probably no big deal!
Okay
> + The per-device <code>boot</code> elements cannot be used
together
> + with general boot elements in
> <a href="#elementsOSBIOS">BIOS bootloader</a>
section.
> <span class="since">Since 0.8.8</span>
> </dd>
> diff --git a/docs/news.xml b/docs/news.xml
> index 52915ee..bd9e43a 100644
> --- a/docs/news.xml
> +++ b/docs/news.xml
> @@ -37,6 +37,17 @@
> <section title="New features">
> <change>
> <summary>
> + qemu: Add support for loadparm for a boot device
> + </summary>
> + <description>
> + Add an optional boot parameter 'loadparm' for a boot device.
> + Loadparm is a 8 byte parameter than can be queried by S390 guests
s/a 8/an 8/
s/can be/when present is/ ?
Yes, thanks for catching that.
> + via sclp or diag 308. Linux guests on S390 use it to select a boot
> + entry.
> + </description>
> + </change>
> + <change>
> + <summary>
> Improved streams to efficiently transfer sparseness
> </summary>
> <description>
The rest seems fine -
John
Thanks for reviewing
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index f88e84a..c885aec 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -5026,6 +5026,13 @@
> <attribute name="order">
> <ref name="positiveInteger"/>
> </attribute>
> + <optional>
> + <attribute name="loadparm">
> + <data type="string">
> + <param name="pattern">[a-zA-Z0-9.\s]{1,8}</param>
> + </data>
> + </attribute>
> + </optional>
> <empty/>
> </element>
> </define>
> diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
> index a20de85..48782be 100644
> --- a/src/conf/device_conf.h
> +++ b/src/conf/device_conf.h
> @@ -167,6 +167,7 @@ struct _virDomainDeviceInfo {
> * assignment, never saved and never reported.
> */
> int pciConnectFlags; /* enum virDomainPCIConnectFlags */
> + char *loadparm;
> };
>
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 8c62673..dbf6108 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3520,6 +3520,7 @@ void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
> memset(&info->addr, 0, sizeof(info->addr));
> info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
> VIR_FREE(info->romfile);
> + VIR_FREE(info->loadparm);
> }
>
>
> @@ -5199,6 +5200,48 @@ virDomainDefValidate(virDomainDefPtr def,
> return 0;
> }
>
> +/**
> + * virDomainDeviceLoadparmIsValid
> + * @loadparm : The string to validate
> + *
> + * The valid set of values for loadparm are [a-zA-Z0-9.]
> + * and blank spaces.
> + * The maximum allowed length is 8 characters.
> + * An empty string is considered invalid
> + */
> +static bool
> +virDomainDeviceLoadparmIsValid(const char *loadparm)
> +{
> + size_t i;
> +
> + if (virStringIsEmpty(loadparm)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("loadparm cannot be an empty string"));
> + return false;
> + }
> +
> + if (strlen(loadparm) > 8) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("loadparm '%s' exceeds 8 characters"),
loadparm);
> + return false;
> + }
> +
> + for (i = 0; i < strlen(loadparm); i++) {
> + uint8_t c = loadparm[i];
> +
> + if (('A' <= c && c <= 'Z') || ('0'
<= c && c <= '9') ||
> + (c == '.') || (c == ' ')) {
> + continue;
> + } else {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("invalid loadparm char '%c', expecting
chars"
> + " in set of [a-zA-Z0-9.] and blank spaces"),
c);
> + return false;
> + }
> + }
> +
> + return true;
> +}
>
> /* Generate a string representation of a device address
> * @info address Device address to stringify
> @@ -5208,9 +5251,14 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
> virDomainDeviceInfoPtr info,
> unsigned int flags)
> {
> - if ((flags & VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) &&
info->bootIndex)
> - virBufferAsprintf(buf, "<boot order='%u'/>\n",
info->bootIndex);
> + if ((flags & VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) &&
info->bootIndex) {
> + virBufferAsprintf(buf, "<boot order='%u'",
info->bootIndex);
>
> + if (info->loadparm)
> + virBufferAsprintf(buf, " loadparm='%s'",
info->loadparm);
> +
> + virBufferAddLit(buf, "/>\n");
> + }
> if (info->alias &&
> !(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) {
> virBufferAsprintf(buf, "<alias name='%s'/>\n",
info->alias);
> @@ -5636,6 +5684,7 @@ virDomainDeviceBootParseXML(xmlNodePtr node,
> virHashTablePtr bootHash)
> {
> char *order;
> + char *loadparm;
> int ret = -1;
>
> if (!(order = virXMLPropString(node, "order"))) {
> @@ -5664,10 +5713,26 @@ virDomainDeviceBootParseXML(xmlNodePtr node,
> goto cleanup;
> }
>
> + loadparm = virXMLPropString(node, "loadparm");
> + if (loadparm) {
> + if (virStringToUpper(&info->loadparm, loadparm) != 1) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to convert loadparm '%s' to upper
case"),
> + loadparm);
> + goto cleanup;
> + }
> +
> + if (!virDomainDeviceLoadparmIsValid(info->loadparm)) {
> + VIR_FREE(info->loadparm);
> + goto cleanup;
> + }
> + }
> +
> ret = 0;
>
> cleanup:
> VIR_FREE(order);
> + VIR_FREE(loadparm);
> return ret;
> }
>
>
8:27 a.m.
New subject: [libvirt] [PATCH v3 2/3] qemu : Add loadparm to qemu command line string
Introduce a new QEMU capability for loadparm and if the capability is
supported by QEMU then append the loadparm value to "-machine" string
of qemu command line.
Signed-off-by: Farhan Ali <alifm(a)linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
Reviewed-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
---
src/qemu/qemu_capabilities.c | 2 ++
src/qemu/qemu_capabilities.h | 1 +
src/qemu/qemu_command.c | 37 +++++++++++++++++++++++++++++++++++++
3 files changed, 40 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 546dfd7..e3bd445 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -371,6 +371,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
"kernel-irqchip.split",
"intel-iommu.intremap",
"intel-iommu.caching-mode",
+ "loadparm",
);
@@ -3144,6 +3145,7 @@ static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] =
{
{ "drive", "throttling.group", QEMU_CAPS_DRIVE_IOTUNE_GROUP },
{ "spice", "rendernode", QEMU_CAPS_SPICE_RENDERNODE },
{ "machine", "kernel_irqchip", QEMU_CAPS_MACHINE_KERNEL_IRQCHIP
},
+ { "machine", "loadparm", QEMU_CAPS_LOADPARM },
};
static int
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index aa99fda..a18c61c 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -409,6 +409,7 @@ typedef enum {
QEMU_CAPS_MACHINE_KERNEL_IRQCHIP_SPLIT, /* -machine kernel_irqchip=split */
QEMU_CAPS_INTEL_IOMMU_INTREMAP, /* intel-iommu.intremap */
QEMU_CAPS_INTEL_IOMMU_CACHING_MODE, /* intel-iommu.caching-mode */
+ QEMU_CAPS_LOADPARM, /* -machine loadparm */
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index aa66e3d..1291b62 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7226,6 +7226,41 @@ qemuAppendKeyWrapMachineParms(virBuffer *buf, virQEMUCapsPtr
qemuCaps,
return true;
}
+static void
+qemuAppendLoadparmMachineParm(virBuffer *buf,
+ const virDomainDef *def,
+ virQEMUCapsPtr qemuCaps)
+{
+ size_t i = 0;
+
+ if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOTINDEX) ||
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_LOADPARM))
+ return;
+
+ for (i = 0; i < def->ndisks; i++) {
+ virDomainDiskDefPtr disk = def->disks[i];
+
+ if (disk->info.bootIndex == 1) {
+ if (disk->info.loadparm)
+ virBufferAsprintf(buf, ",loadparm=%s",
disk->info.loadparm);
+
+ return;
+ }
+ }
+
+ /* Network boot device*/
+ for (i = 0; i < def->nnets; i++) {
+ virDomainNetDefPtr net = def->nets[i];
+
+ if (net->info.bootIndex == 1) {
+ if (net->info.loadparm)
+ virBufferAsprintf(buf, ",loadparm=%s", net->info.loadparm);
+
+ return;
+ }
+ }
+}
+
static int
qemuBuildNameCommandLine(virCommandPtr cmd,
virQEMUDriverConfigPtr cfg,
@@ -7315,6 +7350,8 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
virCommandAddArg(cmd, "-machine");
virBufferAdd(&buf, def->os.machine, -1);
+ qemuAppendLoadparmMachineParm(&buf, def, qemuCaps);
+
if (def->virtType == VIR_DOMAIN_VIRT_QEMU)
virBufferAddLit(&buf, ",accel=tcg");
else if (def->virtType == VIR_DOMAIN_VIRT_KVM)
--
1.9.1
7:34 p.m.
New subject: [libvirt] [PATCH v3 2/3] qemu : Add loadparm to qemu command line string
On 05/23/2017 09:27 AM, Farhan Ali wrote:
Introduce a new QEMU capability for loadparm and if the capability
is
supported by QEMU then append the loadparm value to "-machine" string
of qemu command line.
Signed-off-by: Farhan Ali <alifm(a)linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
Reviewed-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
---
src/qemu/qemu_capabilities.c | 2 ++
src/qemu/qemu_capabilities.h | 1 +
src/qemu/qemu_command.c | 37 +++++++++++++++++++++++++++++++++++++
3 files changed, 40 insertions(+)
I was wondering why there weren't any changes to the tests/
qemucapabilitiesdata/*s390*.xml files... Until I realized this is a
really new feature for qemu... As in post 2.9 and there is no 2.10
*.replies and *.xml file yet created...
The *.xml files add the <flag name=...> entries for whatever qemu
versions the machine loadparm=xxx flag can be found in order to define
the flag to create the .args. But you have .args in v3, which is
confusing (it's late too).
See commit '6b5c6314' for a recently added capability for kernel-irqchip
which modified a number of tests/qemucapabilitiesdata/* output .xml files.
There's also "tests/qemucapsprobe /path/to/binary >caps.replies" which
is what's used to generate those .replies files (those could be added as
part of this or the xml2xml commit).
Sometimes the patch order choice is have patch 1 have all the necessary
flag stuff done, patch 2 the xml/rng/docs, patch 3 the qemu, and patch 4
the news.xml.
diff --git a/src/qemu/qemu_capabilities.c
b/src/qemu/qemu_capabilities.c
index 546dfd7..e3bd445 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -371,6 +371,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
"kernel-irqchip.split",
"intel-iommu.intremap",
"intel-iommu.caching-mode",
+ "loadparm",
);
@@ -3144,6 +3145,7 @@ static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[]
= {
{ "drive", "throttling.group", QEMU_CAPS_DRIVE_IOTUNE_GROUP },
{ "spice", "rendernode", QEMU_CAPS_SPICE_RENDERNODE },
{ "machine", "kernel_irqchip", QEMU_CAPS_MACHINE_KERNEL_IRQCHIP
},
+ { "machine", "loadparm", QEMU_CAPS_LOADPARM },
};
static int
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index aa99fda..a18c61c 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -409,6 +409,7 @@ typedef enum {
QEMU_CAPS_MACHINE_KERNEL_IRQCHIP_SPLIT, /* -machine kernel_irqchip=split */
QEMU_CAPS_INTEL_IOMMU_INTREMAP, /* intel-iommu.intremap */
QEMU_CAPS_INTEL_IOMMU_CACHING_MODE, /* intel-iommu.caching-mode */
+ QEMU_CAPS_LOADPARM, /* -machine loadparm */
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index aa66e3d..1291b62 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7226,6 +7226,41 @@ qemuAppendKeyWrapMachineParms(virBuffer *buf, virQEMUCapsPtr
qemuCaps,
return true;
}
The newer style is two lines between functions
+static void
+qemuAppendLoadparmMachineParm(virBuffer *buf,
+ const virDomainDef *def,
+ virQEMUCapsPtr qemuCaps)
+{
+ size_t i = 0;
+
+ if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOTINDEX) ||
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_LOADPARM))
+ return;
Typically when the flag doesn't exist, we elicit an error message and
cause failure. I'm still scratching my head over how this works for test
when the flags aren't in the XML file, but I'm way too tired to think to
hard about it.
+
+ for (i = 0; i < def->ndisks; i++) {
+ virDomainDiskDefPtr disk = def->disks[i];
+
+ if (disk->info.bootIndex == 1) {
+ if (disk->info.loadparm)
I think you could combine the if conditions... Unless there's thoughts
that other bootIndex's would work... Could end up having arch specific
stuff (yuck).
+ virBufferAsprintf(buf, ",loadparm=%s",
disk->info.loadparm);> +
+ return;
+ }
+ }
+
+ /* Network boot device*/
^
Need a space before end of comment
+ for (i = 0; i < def->nnets; i++) {
+ virDomainNetDefPtr net = def->nets[i];
+
+ if (net->info.bootIndex == 1) {
+ if (net->info.loadparm)
+ virBufferAsprintf(buf, ",loadparm=%s",
net->info.loadparm);
+
+ return;
+ }
+ }
+}
+
Two lines between functions
static int
qemuBuildNameCommandLine(virCommandPtr cmd,
virQEMUDriverConfigPtr cfg,
@@ -7315,6 +7350,8 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
virCommandAddArg(cmd, "-machine");
virBufferAdd(&buf, def->os.machine, -1);
+ qemuAppendLoadparmMachineParm(&buf, def, qemuCaps);
+
Is this required to be the first parameter to -machine? If not then
perhaps it'd be better to place it at the end after the irqchip stuff.
Also, you could move the CapsGet into here so you can keep the void
designation for the called function.
Was there ever thought to adding loadparm to the machine XML? What's the
reasoning to not have it there. If it's only valid for bootindex=1,
then it's far easier to check if the machine XML has it defined rather
than perusing the disk/network lists (which could be lengthy) only to
fine none. If the concern is some other arch allowing a different
bootindex to have loadparm, then the simple decision there is to have a
"loadparm_bootindex=#" value that would match the disk bootindex=# value.
Tks -
John
if (def->virtType == VIR_DOMAIN_VIRT_QEMU)
virBufferAddLit(&buf, ",accel=tcg");
else if (def->virtType == VIR_DOMAIN_VIRT_KVM)
10 a.m.
New subject: [libvirt] [PATCH v3 2/3] qemu : Add loadparm to qemu command line string
On 05/25/2017 08:34 PM, John Ferlan wrote:
On 05/23/2017 09:27 AM, Farhan Ali wrote:
> Introduce a new QEMU capability for loadparm and if the capability is
> supported by QEMU then append the loadparm value to "-machine" string
> of qemu command line.
>
> Signed-off-by: Farhan Ali <alifm(a)linux.vnet.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
> Reviewed-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
> ---
> src/qemu/qemu_capabilities.c | 2 ++
> src/qemu/qemu_capabilities.h | 1 +
> src/qemu/qemu_command.c | 37 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 40 insertions(+)
>
I was wondering why there weren't any changes to the tests/
qemucapabilitiesdata/*s390*.xml files... Until I realized this is a
really new feature for qemu... As in post 2.9 and there is no 2.10
*.replies and *.xml file yet created...
The *.xml files add the <flag name=...> entries for whatever qemu
versions the machine loadparm=xxx flag can be found in order to define
the flag to create the .args. But you have .args in v3, which is
confusing (it's late too).
See commit '6b5c6314' for a recently added capability for kernel-irqchip
which modified a number of tests/qemucapabilitiesdata/* output .xml files.
There's also "tests/qemucapsprobe /path/to/binary >caps.replies" which
is what's used to generate those .replies files (those could be added as
part of this or the xml2xml commit).
Will add the relevant files for s390x qemu 2.10
Sometimes the patch order choice is have patch 1 have all the necessary
flag stuff done, patch 2 the xml/rng/docs, patch 3 the qemu, and patch 4
the news.xml.
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 546dfd7..e3bd445 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -371,6 +371,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
> "kernel-irqchip.split",
> "intel-iommu.intremap",
> "intel-iommu.caching-mode",
> + "loadparm",
> );
>
>
> @@ -3144,6 +3145,7 @@ static struct virQEMUCapsCommandLineProps
virQEMUCapsCommandLine[] = {
> { "drive", "throttling.group", QEMU_CAPS_DRIVE_IOTUNE_GROUP
},
> { "spice", "rendernode", QEMU_CAPS_SPICE_RENDERNODE },
> { "machine", "kernel_irqchip",
QEMU_CAPS_MACHINE_KERNEL_IRQCHIP },
> + { "machine", "loadparm", QEMU_CAPS_LOADPARM },
> };
>
> static int
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index aa99fda..a18c61c 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -409,6 +409,7 @@ typedef enum {
> QEMU_CAPS_MACHINE_KERNEL_IRQCHIP_SPLIT, /* -machine kernel_irqchip=split */
> QEMU_CAPS_INTEL_IOMMU_INTREMAP, /* intel-iommu.intremap */
> QEMU_CAPS_INTEL_IOMMU_CACHING_MODE, /* intel-iommu.caching-mode */
> + QEMU_CAPS_LOADPARM, /* -machine loadparm */
>
> QEMU_CAPS_LAST /* this must always be the last item */
> } virQEMUCapsFlags;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index aa66e3d..1291b62 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7226,6 +7226,41 @@ qemuAppendKeyWrapMachineParms(virBuffer *buf, virQEMUCapsPtr
qemuCaps,
> return true;
> }
>
The newer style is two lines between functions
Sure, wasn't aware of this. Thanks
> +static void
> +qemuAppendLoadparmMachineParm(virBuffer *buf,
> + const virDomainDef *def,
> + virQEMUCapsPtr qemuCaps)
> +{
> + size_t i = 0;
> +
> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOTINDEX) ||
> + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_LOADPARM))
> + return;
Typically when the flag doesn't exist, we elicit an error message and
cause failure. I'm still scratching my head over how this works for test
when the flags aren't in the XML file, but I'm way too tired to think to
hard about it.
> +
> + for (i = 0; i < def->ndisks; i++) {
> + virDomainDiskDefPtr disk = def->disks[i];
> +
> + if (disk->info.bootIndex == 1) {
> + if (disk->info.loadparm)
I think you could combine the if conditions... Unless there's thoughts
that other bootIndex's would work... Could end up having arch specific
stuff (yuck).
> + virBufferAsprintf(buf, ",loadparm=%s",
disk->info.loadparm);> +
> + return;
> + }
> + }
> +
> + /* Network boot device*/
^
Need a space before end of comment
> + for (i = 0; i < def->nnets; i++) {
> + virDomainNetDefPtr net = def->nets[i];
> +
> + if (net->info.bootIndex == 1) {
> + if (net->info.loadparm)
> + virBufferAsprintf(buf, ",loadparm=%s",
net->info.loadparm);
> +
> + return;
> + }
> + }
> +}
> +
Two lines between functions
> static int
> qemuBuildNameCommandLine(virCommandPtr cmd,
> virQEMUDriverConfigPtr cfg,
> @@ -7315,6 +7350,8 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
> virCommandAddArg(cmd, "-machine");
> virBufferAdd(&buf, def->os.machine, -1);
>
> + qemuAppendLoadparmMachineParm(&buf, def, qemuCaps);
> +
Is this required to be the first parameter to -machine? If not then
perhaps it'd be better to place it at the end after the irqchip stuff.
Sure.
Also, you could move the CapsGet into here so you can keep the void
designation for the called function.
okay
Was there ever thought to adding loadparm to the machine XML?
What's the
reasoning to not have it there. If it's only valid for bootindex=1,
then it's far easier to check if the machine XML has it defined rather
than perusing the disk/network lists (which could be lengthy) only to
fine none. If the concern is some other arch allowing a different
bootindex to have loadparm, then the simple decision there is to have a
"loadparm_bootindex=#" value that would match the disk bootindex=# value.
I am not sure what you mean here? By machine xml do you mean <os> xml?
Tks -
John
> if (def->virtType == VIR_DOMAIN_VIRT_QEMU)
> virBufferAddLit(&buf, ",accel=tcg");
> else if (def->virtType == VIR_DOMAIN_VIRT_KVM)
>
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
3:03 p.m.
New subject: [libvirt] [PATCH v3 2/3] qemu : Add loadparm to qemu command line string
...
> Was there ever thought to adding loadparm to the machine XML?
What's the
> reasoning to not have it there. If it's only valid for bootindex=1,
> then it's far easier to check if the machine XML has it defined rather
> than perusing the disk/network lists (which could be lengthy) only to
> fine none. If the concern is some other arch allowing a different
> bootindex to have loadparm, then the simple decision there is to have a
> "loadparm_bootindex=#" value that would match the disk bootindex=# value.
>
I am not sure what you mean here? By machine xml do you mean <os> xml?
Sorry I was too lazy to go make the cross reference near the end of the
day/review. Guess I was thinking more <os> related though...
I see in <os> there's a <boot> subelement which has a relationship with
the <boot> subelement for <devices>...[<interface>|<disk>]...
I think I was just trying to make sure that adding <loadparm> to
<devices> would make sense "in the long run" and what other
implementations were considered and not taken because of some drawback.
Given the description I've read and the implementation that searches
either disk or network lists looking for bootIndex = 1, I wonder if the
<os> <boot dev='xxx' > should be modified instead. Was that
considered
- what were the drawbacks? Can bootparm conceptually be added/used for
a non boot disk?
I'm not requesting one way or another - I'd just like to be sure the
question is answered before perhaps someone else asks it now or much
later when this isn't so fresh in your mind.
John
9:27 a.m.
New subject: [libvirt] [PATCH v3 2/3] qemu : Add loadparm to qemu command line string
On 05/26/2017 04:03 PM, John Ferlan wrote:
...
>> Was there ever thought to adding loadparm to the machine XML? What's the
>> reasoning to not have it there. If it's only valid for bootindex=1,
>> then it's far easier to check if the machine XML has it defined rather
>> than perusing the disk/network lists (which could be lengthy) only to
>> fine none. If the concern is some other arch allowing a different
>> bootindex to have loadparm, then the simple decision there is to have a
>> "loadparm_bootindex=#" value that would match the disk bootindex=#
value.
>>
>
> I am not sure what you mean here? By machine xml do you mean <os> xml?
>
Sorry I was too lazy to go make the cross reference near the end of the
day/review. Guess I was thinking more <os> related though...
I see in <os> there's a <boot> subelement which has a relationship with
the <boot> subelement for <devices>...[<interface>|<disk>]...
Yes, the <os> has <boot> subelement, but it can be tricky to specify the
order of the boot device.
I think I was just trying to make sure that adding <loadparm>
to
<devices> would make sense "in the long run" and what other
implementations were considered and not taken because of some drawback.
The per device boot subelement was added to simplify the boot device
order. That's why I thought it made more sense to add it to the per
device boot element. Also from a user's perspective it's easier to
specify the loadparm when specifying the boot order.
Given the description I've read and the implementation that
searches
either disk or network lists looking for bootIndex = 1, I wonder if the
<os> <boot dev='xxx' > should be modified instead. Was that
considered
- what were the drawbacks? Can bootparm conceptually be added/used for
a non boot disk?
I'm not requesting one way or another - I'd just like to be sure the
question is answered before perhaps someone else asks it now or much
later when this isn't so fresh in your mind.
John
8:27 a.m.
New subject: [libvirt] [PATCH v3 3/3] tests : Testcases for loadparm
Add testcases for loadparm
Signed-off-by: Farhan Ali <alifm(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
Reviewed-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
---
...-machine-loadparm-multiple-disks-nets-s390.args | 28 ++++++++++++++
...v-machine-loadparm-multiple-disks-nets-s390.xml | 43 ++++++++++++++++++++++
.../qemuxml2argv-machine-loadparm-net-s390.args | 20 ++++++++++
.../qemuxml2argv-machine-loadparm-net-s390.xml | 26 +++++++++++++
...xml2argv-machine-loadparm-s390-char-invalid.xml | 26 +++++++++++++
...uxml2argv-machine-loadparm-s390-len-invalid.xml | 26 +++++++++++++
.../qemuxml2argv-machine-loadparm-s390.args | 20 ++++++++++
.../qemuxml2argv-machine-loadparm-s390.xml | 26 +++++++++++++
tests/qemuxml2argvtest.c | 19 ++++++++++
9 files changed, 234 insertions(+)
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-multiple-disks-nets-s390.args
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-multiple-disks-nets-s390.xml
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-net-s390.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-net-s390.xml
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-s390-char-invalid.xml
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-s390-len-invalid.xml
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-s390.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-s390.xml
diff --git
a/tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-multiple-disks-nets-s390.args
b/tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-multiple-disks-nets-s390.args
new file mode 100644
index 0000000..c9e9061
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-multiple-disks-nets-s390.args
@@ -0,0 +1,28 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-s390x \
+-name QEMUGuest1 \
+-S \
+-machine s390-ccw-virtio,loadparm=SYSTEM1,accel=tcg \
+-m 214 \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-nographic \
+-nodefaults \
+-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \
+-device virtio-blk-ccw,devno=fe.0.0002,drive=drive-virtio-disk0,\
+id=virtio-disk0,bootindex=1 \
+-drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-virtio-disk1 \
+-device virtio-blk-ccw,devno=fe.0.0003,drive=drive-virtio-disk1,\
+id=virtio-disk1,bootindex=3 \
+-device virtio-net-ccw,vlan=0,id=net0,mac=00:11:22:33:44:54,devno=fe.0.0000,\
+bootindex=2 \
+-net user,vlan=0,name=hostnet0 \
+-device virtio-net-ccw,vlan=1,id=net1,mac=00:11:22:33:42:36,devno=fe.0.0004 \
+-net user,vlan=1,name=hostnet1 \
+-device virtio-balloon-ccw,id=balloon0,devno=fe.0.0001
diff --git
a/tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-multiple-disks-nets-s390.xml
b/tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-multiple-disks-nets-s390.xml
new file mode 100644
index 0000000..df442e9
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-multiple-disks-nets-s390.xml
@@ -0,0 +1,43 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory>219136</memory>
+ <currentMemory>219136</currentMemory>
+ <vcpu>1</vcpu>
+ <os>
+ <type arch='s390x' machine='s390-ccw-virtio'>hvm</type>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-s390x</emulator>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='virtio'/>
+ <boot order='1' loadparm='system1'/>
+ <address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x2'/>
+ </disk>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest2'/>
+ <target dev='hdb' bus='virtio'/>
+ <boot order='3' loadparm='3'/>
+ <address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x3'/>
+ </disk>
+ <interface type='user'>
+ <mac address='00:11:22:33:44:54'/>
+ <model type='virtio'/>
+ <boot order='2' loadparm='2.lp'/>
+ <address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x0'/>
+ </interface>
+ <interface type='user'>
+ <mac address='00:11:22:33:42:36'/>
+ <model type='virtio'/>
+ <address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x4'/>
+ </interface>
+ <memballoon model='virtio'>
+ <address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x1'/>
+ </memballoon>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-net-s390.args
b/tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-net-s390.args
new file mode 100644
index 0000000..4ca2865
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-net-s390.args
@@ -0,0 +1,20 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-s390x \
+-name QEMUGuest1 \
+-S \
+-machine s390-ccw-virtio,loadparm=2,accel=tcg \
+-m 214 \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-nographic \
+-nodefaults \
+-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \
+-device virtio-net-ccw,vlan=0,id=net0,mac=00:11:22:33:44:54,devno=fe.0.0000,\
+bootindex=1 \
+-net user,vlan=0,name=hostnet0 \
+-device virtio-balloon-ccw,id=balloon0,devno=fe.0.0001
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-net-s390.xml
b/tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-net-s390.xml
new file mode 100644
index 0000000..ca682af
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-net-s390.xml
@@ -0,0 +1,26 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory>219136</memory>
+ <currentMemory>219136</currentMemory>
+ <vcpu>1</vcpu>
+ <os>
+ <type arch='s390x' machine='s390-ccw-virtio'>hvm</type>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-s390x</emulator>
+ <interface type='user'>
+ <mac address='00:11:22:33:44:54'/>
+ <model type='virtio'/>
+ <boot order='1' loadparm='2'/>
+ <address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x0'/>
+ </interface>
+ <memballoon model='virtio'>
+ <address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x1'/>
+ </memballoon>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-s390-char-invalid.xml
b/tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-s390-char-invalid.xml
new file mode 100644
index 0000000..5cca5f9
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-s390-char-invalid.xml
@@ -0,0 +1,26 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory>219136</memory>
+ <currentMemory>219136</currentMemory>
+ <vcpu>1</vcpu>
+ <os>
+ <type arch='s390x' machine='s390-ccw-virtio'>hvm</type>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-s390x</emulator>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='virtio'/>
+ <boot order='1' loadparm='sys1?'/>
+ <address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x0'/>
+ </disk>
+ <memballoon model='virtio'>
+ <address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x1'/>
+ </memballoon>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-s390-len-invalid.xml
b/tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-s390-len-invalid.xml
new file mode 100644
index 0000000..ddc4baa
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-s390-len-invalid.xml
@@ -0,0 +1,26 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory>219136</memory>
+ <currentMemory>219136</currentMemory>
+ <vcpu>1</vcpu>
+ <os>
+ <type arch='s390x' machine='s390-ccw-virtio'>hvm</type>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-s390x</emulator>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='virtio'/>
+ <boot order='1' loadparm='loadparm1'/>
+ <address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x0'/>
+ </disk>
+ <memballoon model='virtio'>
+ <address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x1'/>
+ </memballoon>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-s390.args
b/tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-s390.args
new file mode 100644
index 0000000..34855e9
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-s390.args
@@ -0,0 +1,20 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-s390x \
+-name QEMUGuest1 \
+-S \
+-machine s390-ccw-virtio,loadparm=2,accel=tcg \
+-m 214 \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-nographic \
+-nodefaults \
+-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \
+-device virtio-blk-ccw,devno=fe.0.0000,drive=drive-virtio-disk0,\
+id=virtio-disk0,bootindex=1 \
+-device virtio-balloon-ccw,id=balloon0,devno=fe.0.0001
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-s390.xml
b/tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-s390.xml
new file mode 100644
index 0000000..96db020
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-s390.xml
@@ -0,0 +1,26 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory>219136</memory>
+ <currentMemory>219136</currentMemory>
+ <vcpu>1</vcpu>
+ <os>
+ <type arch='s390x' machine='s390-ccw-virtio'>hvm</type>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-s390x</emulator>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='virtio'/>
+ <boot order='1' loadparm='2'/>
+ <address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x0'/>
+ </disk>
+ <memballoon model='virtio'>
+ <address type='ccw' cssid='0xfe' ssid='0x0'
devno='0x1'/>
+ </memballoon>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 4269598..690cd55 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2460,6 +2460,25 @@ mymain(void)
QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_VIRTIO_SCSI,
QEMU_CAPS_VIRTIO_CCW, QEMU_CAPS_VIRTIO_S390);
+ DO_TEST("machine-loadparm-s390", QEMU_CAPS_MACHINE_OPT,
+ QEMU_CAPS_VIRTIO_CCW, QEMU_CAPS_VIRTIO_S390,
+ QEMU_CAPS_BOOTINDEX, QEMU_CAPS_LOADPARM);
+ DO_TEST("machine-loadparm-net-s390", QEMU_CAPS_MACHINE_OPT,
+ QEMU_CAPS_VIRTIO_CCW, QEMU_CAPS_VIRTIO_S390,
+ QEMU_CAPS_BOOTINDEX, QEMU_CAPS_LOADPARM);
+ DO_TEST("machine-loadparm-multiple-disks-nets-s390",
+ QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_VIRTIO_CCW,
+ QEMU_CAPS_VIRTIO_S390, QEMU_CAPS_BOOTINDEX,
+ QEMU_CAPS_LOADPARM);
+ DO_TEST_PARSE_ERROR("machine-loadparm-s390-char-invalid",
+ QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_VIRTIO_CCW,
+ QEMU_CAPS_VIRTIO_S390, QEMU_CAPS_BOOTINDEX,
+ QEMU_CAPS_LOADPARM);
+ DO_TEST_PARSE_ERROR("machine-loadparm-s390-len-invalid",
+ QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_VIRTIO_CCW,
+ QEMU_CAPS_VIRTIO_S390, QEMU_CAPS_BOOTINDEX,
+ QEMU_CAPS_LOADPARM);
+
DO_TEST("qemu-ns-domain-ns0", NONE);
DO_TEST("qemu-ns-domain-commandline", NONE);
DO_TEST("qemu-ns-domain-commandline-ns0", NONE);
--
1.9.1
7:35 p.m.
New subject: [libvirt] [PATCH v3 3/3] tests : Testcases for loadparm
On 05/23/2017 09:27 AM, Farhan Ali wrote:
Add testcases for loadparm
Signed-off-by: Farhan Ali <alifm(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
Reviewed-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
---
...-machine-loadparm-multiple-disks-nets-s390.args | 28 ++++++++++++++
...v-machine-loadparm-multiple-disks-nets-s390.xml | 43 ++++++++++++++++++++++
.../qemuxml2argv-machine-loadparm-net-s390.args | 20 ++++++++++
.../qemuxml2argv-machine-loadparm-net-s390.xml | 26 +++++++++++++
...xml2argv-machine-loadparm-s390-char-invalid.xml | 26 +++++++++++++
...uxml2argv-machine-loadparm-s390-len-invalid.xml | 26 +++++++++++++
.../qemuxml2argv-machine-loadparm-s390.args | 20 ++++++++++
.../qemuxml2argv-machine-loadparm-s390.xml | 26 +++++++++++++
tests/qemuxml2argvtest.c | 19 ++++++++++
9 files changed, 234 insertions(+)
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-multiple-disks-nets-s390.args
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-multiple-disks-nets-s390.xml
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-net-s390.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-net-s390.xml
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-s390-char-invalid.xml
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-s390-len-invalid.xml
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-s390.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-s390.xml
These all seem reasonable - but could be merged with patch2 which had
the qemu changes.
John
2733
days inactive
2740
days old
10 comments
2 participants
participants (2)
-
Farhan Ali -
John Ferlan