5:58 a.m.
On 07/05/13 02:01, John Ferlan wrote:
On 05/03/2013 02:07 PM, Osier Yang wrote:
> Since it's generic enough to be used by other types in future, I
> put it in <hostdev> as sub-element, though now it's only used by
> scsi host device.
> ---
> docs/formatdomain.html.in | 4 +++
> docs/schemas/domaincommon.rng | 5 +++
> src/conf/domain_conf.c | 6 ++++
> src/conf/domain_conf.h | 1 +
> src/qemu/qemu_command.c | 4 +++
> .../qemuxml2argv-hostdev-scsi-readonly.args | 9 ++++++
> .../qemuxml2argv-hostdev-scsi-readonly.xml | 36 ++++++++++++++++++++++
> tests/qemuxml2argvtest.c | 4 +++
> tests/qemuxml2xmltest.c | 1 +
> 9 files changed, 70 insertions(+)
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-readonly.args
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-readonly.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 488673f..9cd79e5 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -2406,6 +2406,10 @@
> could be changed in the future with no impact to domains that
> don't specify anything.
> </dd>
> + <dt><code>readonly</code></dt>
> + <dd>Indicates that the device is readonly, only supported by SCSI host
> + device now. <span class="since">Since 1.0.6</span>
> + </dd>
I just realized your example in 1/25 has the 'readonly' element listed
in the new "<pre> ... </pre> section, but it's not described. I
think
you probably need to split that line out of there and then put it into
this patch.
Yeah, I missed it, should remove it in 1/25.
Does anything need to be said here that the underlying QEMU needs to
support the readonly too? That is some version or capability? Since
there is a check later on to actually pass/add readonly to the command
line, it'd surely raise a question some day if someone defined it as
readonly, but then come to find out the domain didn't honor that...
Agreed. QEMU/KVM only.
> </dl>
>
>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index b8d0d60..4fdacab 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -3075,6 +3075,11 @@
> <optional>
> <ref name="address"/>
> </optional>
> + <optional>
> + <element name="readonly">
> + <empty/>
> + </element>
> + </optional>
> </interleave>
> </element>
> </define>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 9e6b65b..31a8c46 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -8724,6 +8724,9 @@ virDomainHostdevDefParseXML(const xmlNodePtr node,
> _("SCSI host devices must have address
specified"));
> goto error;
> }
> +
> + if (virXPathBoolean("boolean(./readonly)", ctxt))
> + def->readonly = true;
> break;
> }
> }
> @@ -15342,6 +15345,9 @@ virDomainHostdevDefFormat(virBufferPtr buf,
> return -1;
> break;
> }
> +
> + if (def->readonly)
> + virBufferAddLit(buf, "<readonly/>\n");
> virBufferAdjustIndent(buf, -6);
>
> if (virDomainDeviceInfoFormat(buf, def->info,
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 1efae69..5471cd3 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -461,6 +461,7 @@ struct _virDomainHostdevDef {
> int startupPolicy; /* enum virDomainStartupPolicy */
> bool managed;
> bool missing;
> + bool readonly;
> union {
> virDomainHostdevSubsys subsys;
> virDomainHostdevCaps caps;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index df896aa..2b141d1 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4702,6 +4702,10 @@ qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev,
> virDomainDeviceAddressTypeToString(dev->info->type),
> dev->info->alias);
>
> + if (dev->readonly &&
> + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_READONLY))
> + virBufferAsprintf(&buf, ",readonly=on");
> +
See comment above. If this is defined as readonly, but the underlying
system doesn't support it, then should we fail? It'd be bad news to
have a domain expect something readonly, but still load and potentially
write on something it wasn't supposed to.
If there's a precedent already that allows a defined readonly element to
be used read/write, then I guess it's a weak ACK, but otherwise, I think
it might be best to fail.
We don't have a standdard yet, some attributes are just ignored if
the underlying hypervisor doesn't support it, some will fail. Unfortunately,
we can't change it simply to error out, as it will introduce "regression"
from user's p.o.v, though from out p.o.v, it's improvement.
I will let it fail when pushing.
Osier