[libvirt] [PATCH 06/25] Introduce <readonly> for hostdev

[Osier Yang]

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.