On 07/31/2013 09:51 AM, Guannan Ren wrote:
> Add startupPolicy attribute policy for harddisk with type "file",
s/policy//
> "block" and "dir". 'requisite' is not supported currently for
> hardisk.
s/hardisk/harddisk/
> ---
> docs/formatdomain.html.in | 8 ++++++--
> docs/schemas/domaincommon.rng | 6 ++++++
> src/conf/domain_conf.c | 31 +++++++++++++++++++++++--------
> 3 files changed, 35 insertions(+), 10 deletions(-)
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 78e132e..cbfc773 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1620,7 +1620,8 @@
> policy what to do with the disk if the source file is not accessible.
> (NB, <code>startupPolicy</code> is not valid for "volume" disk unless
> the specified storage volume is of "file" type). This is done by the
> - <code>startupPolicy</code> attribute, accepting these values:
> + <code>startupPolicy</code> attribute(<span class="since">Since 0.9.7</span>),
Add a space before the opening bracket.
> + accepting these values:
> <table class="top_table">
> <tr>
> <td> mandatory </td>
> @@ -1636,7 +1637,10 @@
> <td> drop if missing at any start attempt </td>
> </tr>
> </table>
> - <span class="since">Since 0.9.7</span>
> + <span class="since">Since 1.1.2</span> the <code>startupPolicy</code> is extended
> + to support hard disks besides cdrom and floppy. On guest cold bootup, if a certain disk
> + is not accessible or its disk chain is broken, with startupPolicy 'optional' the guest
> + will drop this disk. This feature doesn't support migration currently.
I'm worried that in case there is the possibility to (un)plug the disks
during migration appears in the future, and we make a use of that, we'd
have to stick with the same behavior for 'optional' policy which is
described here in order not to be backwards compatible. It may be
confusing to some users as well. What is your (and others) opinion on
adding another startupPolicy called for example 'drop_on_boot' (or
something more meaningful and appropriate) which would mean exactly the
behavior explained here?
I must say that I'm complete OK with this approach in case we're sure
that we will never have the possibility to have automatically
(un)pluggable disks during migration. And I personally, think, that
this won't happen on qemu/libvirt level.
It would be nice if others expressed their opinion on this as well.
> </dd>
> <dt><code>mirror</code></dt>
> <dd>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 745b959..c6ee01c 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1124,6 +1124,9 @@
> <ref name="absFilePath"/>
> </attribute>
> <optional>
> + <ref name="startupPolicy"/>
> + </optional>
> + <optional>
> <ref name='devSeclabel'/>
> </optional>
> </element>
> @@ -1141,6 +1144,9 @@
> <attribute name="dir">
> <ref name="absFilePath"/>
> </attribute>
> + <optional>
> + <ref name="startupPolicy"/>
> + </optional>
> <empty/>
> </element>
> </optional>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index a86be8c..e764492 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -4795,7 +4795,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
> switch (def->type) {
> case VIR_DOMAIN_DISK_TYPE_FILE:
> source = virXMLPropString(cur, "file");
> - startupPolicy = virXMLPropString(cur, "startupPolicy");
> break;
> case VIR_DOMAIN_DISK_TYPE_BLOCK:
> source = virXMLPropString(cur, "dev");
> @@ -4882,7 +4881,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
> case VIR_DOMAIN_DISK_TYPE_VOLUME:
> if (virDomainDiskSourcePoolDefParse(cur, def) < 0)
> goto error;
> - startupPolicy = virXMLPropString(cur, "startupPolicy");
> break;
> default:
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -4891,6 +4889,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
> goto error;
> }
>
> + startupPolicy = virXMLPropString(cur, "startupPolicy");
> +
> /* People sometimes pass a bogus '' source path
> when they mean to omit the source element
> completely (e.g. CDROM without media). This is
> @@ -5463,14 +5463,22 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
> goto error;
> }
>
> - if (def->device != VIR_DOMAIN_DISK_DEVICE_CDROM &&
> - def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
> - virReportError(VIR_ERR_INVALID_ARG,
> - _("Setting disk %s is allowed only for "
> - "cdrom or floppy"),
> + if (def->type == VIR_DOMAIN_DISK_TYPE_NETWORK) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Setting disk %s is not allowed for "
> + "disk of network type"),
> startupPolicy);
> goto error;
> }
> +
> + if (def->device != VIR_DOMAIN_DISK_DEVICE_CDROM &&
> + def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY &&
> + val == VIR_DOMAIN_STARTUP_POLICY_REQUISITE) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("Setting disk 'requisite' only for "
You deleted "is allowed" from the message.
> + "cdrom or floppy"));
> + goto error;
> + }
> def->startupPolicy = val;
> }
>
> @@ -14125,6 +14133,9 @@ virDomainDiskSourceDefFormat(virBufferPtr buf,
> case VIR_DOMAIN_DISK_TYPE_BLOCK:
> virBufferEscapeString(buf, " <source dev='%s'",
> def->src);
> + if (def->startupPolicy)
> + virBufferEscapeString(buf, " startupPolicy='%s'",
> + startupPolicy);
> if (def->nseclabels) {
> virBufferAddLit(buf, ">\n");
> virBufferAdjustIndent(buf, 8);
> @@ -14137,8 +14148,12 @@ virDomainDiskSourceDefFormat(virBufferPtr buf,
> }
> break;
> case VIR_DOMAIN_DISK_TYPE_DIR:
> - virBufferEscapeString(buf, " <source dir='%s'/>\n",
> + virBufferEscapeString(buf, " <source dir='%s'",
> def->src);
> + if (def->startupPolicy)
> + virBufferEscapeString(buf, " startupPolicy='%s'",
> + startupPolicy);
> + virBufferAddLit(buf, "/>\n");
> break;
> case VIR_DOMAIN_DISK_TYPE_NETWORK:
> virBufferAsprintf(buf, " <source protocol='%s'",
>
Not that it would happen, but after this patch and before the folowing
one, the code is inconsistent, the next patch should be squashed in this
one. ACK with the nits fixed and the next one squashed in.
Martin
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list