6:04 a.m.
On 07/05/13 18:58, Osier Yang wrote:
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.
Pushed with the nits fixed.