On 2013年03月06日 20:38, Han Cheng wrote:
> Thanks for your comments~ Please correct me if I'm wrong.
>
> As I know,<source> in<hostdev> is parsed by
> virDomainHostdevSubsys(Pci/Usb)DefParseXML. Everything else in<hostdev>
> is parsed by virDomainDeviceInfoParseXML.
> I add readonly follow this.
I do think the new "readonly" member should be inside
virDomainHostdevDef instead. On one hand, not all devices
that has virDomainDeviceInfo want to support "readonly".
On the other hand, see what virDomainDiskDef and virDomainFSDef
does. if you add it in virDomainDeviceInfo, for disk, fs,
(etc) devices, they are just duplicate.
And what I mean for "exposing the readonly" is to make
it external, but not internal instead, I.E. when you
dump the XML, you should see it.
>
> And this XML is tested by hostdev-scsi-readonly(named by your advice).
>
> Other problems will be fixed by next version.
>
> On 03/06/2013 01:40 PM, Osier Yang wrote:
>> On 2013年03月04日 14:01, Han Cheng wrote:
>>> The only parameter in -drive affect scsi-generic is readonly. Introduce
>>> <readonly/> to<hostdev>.
>>> The helper function to look up disk controller model may be used by scsi
>>> hostdev. But it should be changed to use info.
>>> ---
>>> docs/schemas/domaincommon.rng | 5 +++++
>>> src/conf/domain_conf.c | 18 ++++++++++++++----
>>> src/conf/domain_conf.h | 6 ++++--
>>> src/libvirt_private.syms | 2 +-
>>> src/qemu/qemu_command.c | 4 ++--
>>> 5 files changed, 26 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
>>> index e7231cc..fbb4001 100644
>>> --- a/docs/schemas/domaincommon.rng
>>> +++ b/docs/schemas/domaincommon.rng
>>> @@ -2898,6 +2898,11 @@
>>> <ref name="alias"/>
>>> </optional>
>>> <optional>
>>> +<element name='readonly'>
>>> +<empty/>
>>> +</element>
>>> +</optional>
>>> +<optional>
>>> <ref name="deviceBoot"/>
>>> </optional>
>>> <optional>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index 995cf0c..5e385e4 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -78,6 +78,7 @@ typedef enum {
>>> VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES = (1<<18),
>>> VIR_DOMAIN_XML_INTERNAL_ALLOW_ROM = (1<<19),
>>> VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT = (1<<20),
>>> + VIR_DOMAIN_XML_INTERNAL_ALLOW_READONLY = (1<<21),
>>
>> I think the "readonly" tag for hostdev can just always be exposed,
>> as don't see any special reason to keep it internally.
>>
>>> } virDomainXMLInternalFlags;
>>>
>>> VIR_ENUM_IMPL(virDomainTaint, VIR_DOMAIN_TAINT_LAST,
>>> @@ -2173,6 +2174,8 @@ virDomainDeviceInfoIsSet(virDomainDeviceInfoPtr info, unsigned int flags)
>>> return true;
>>> if (info->bootIndex)
>>> return true;
>>> + if (info->readonly)
>>> + return true;
>>
>> And why it's of DeviceInfo struct?
>>
>>> return false;
>>> }
>>>
>>> @@ -2395,6 +2398,8 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
>>> virDomainDeviceInfoPtr info,
>>> unsigned int flags)
>>> {
>>> + if ((flags& VIR_DOMAIN_XML_INTERNAL_ALLOW_READONLY)&& info->readonly)
>>> + virBufferAsprintf(buf, "<readonly/>\n");
>>> if ((flags& VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT)&& info->bootIndex)
>>> virBufferAsprintf(buf, "<boot order='%d'/>\n", info->bootIndex);
>>>
>>> @@ -2803,6 +2808,10 @@ virDomainDeviceInfoParseXML(xmlNodePtr node,
>>> (flags& VIR_DOMAIN_XML_INTERNAL_ALLOW_ROM)&&
>>> xmlStrEqual(cur->name, BAD_CAST "rom")) {
>>> rom = cur;
>>> + } else if (info->readonly == 0&&
>>> + (flags& VIR_DOMAIN_XML_INTERNAL_ALLOW_READONLY)&&
>>> + xmlStrEqual(cur->name, BAD_CAST "readonly")) {
>>> + info->readonly = 1;
>>> }
>>> }
>>> cur = cur->next;
>>> @@ -3291,8 +3300,8 @@ error:
>>> }
>>>
>>> int
>>> -virDomainDiskFindControllerModel(virDomainDefPtr def,
>>> - virDomainDiskDefPtr disk,
>>> +virDomainInfoFindControllerModel(virDomainDefPtr def,
>>
>> Not a good name. How about changing into:
>>
>> virDomainDeviceFindControllerModel.
>>
>>> + virDomainDeviceInfoPtr info,
>>> int controllerType)
>>> {
>>> int model = -1;
>>> @@ -3300,7 +3309,7 @@ virDomainDiskFindControllerModel(virDomainDefPtr def,
>>>
>>> for (i = 0; i< def->ncontrollers; i++) {
>>> if (def->controllers[i]->type == controllerType&&
>>> - def->controllers[i]->idx == disk->info.addr.drive.controller)
>>> + def->controllers[i]->idx == info->addr.drive.controller)
>>> model = def->controllers[i]->model;
>>> }
>>>
>>> @@ -7838,7 +7847,8 @@ virDomainHostdevDefParseXML(const xmlNodePtr node,
>>> if (def->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
>>> if (virDomainDeviceInfoParseXML(node, bootMap, def->info,
>>> flags | VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT
>>> - | VIR_DOMAIN_XML_INTERNAL_ALLOW_ROM)< 0)
>>> + | VIR_DOMAIN_XML_INTERNAL_ALLOW_ROM
>>> + | VIR_DOMAIN_XML_INTERNAL_ALLOW_READONLY)< 0)
>>> goto error;
>>> }
>>>
>>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>>> index 5828ae2..39c5849 100644
>>> --- a/src/conf/domain_conf.h
>>> +++ b/src/conf/domain_conf.h
>>> @@ -286,6 +286,8 @@ struct _virDomainDeviceInfo {
>>> /* bootIndex is only used for disk, network interface, hostdev
>>> * and redirdev devices */
>>> int bootIndex;
>>> + /* readonly is only used for scsi hostdev */
>>> + int readonly;
>>
>> This should be a member of virDomainHostdevDef instead.
>>
>>> };
>>>
>>> enum virDomainSeclabelType {
>>> @@ -1951,8 +1953,8 @@ void virDomainInputDefFree(virDomainInputDefPtr def);
>>> void virDomainDiskDefFree(virDomainDiskDefPtr def);
>>> void virDomainLeaseDefFree(virDomainLeaseDefPtr def);
>>> void virDomainDiskHostDefFree(virDomainDiskHostDefPtr def);
>>> -int virDomainDiskFindControllerModel(virDomainDefPtr def,
>>> - virDomainDiskDefPtr disk,
>>> +int virDomainInfoFindControllerModel(virDomainDefPtr def,
>>> + virDomainDeviceInfoPtr info,
>>> int controllerType);
>>> virDomainDiskDefPtr virDomainDiskFindByBusAndDst(virDomainDefPtr def,
>>> int bus,
>>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>>> index c7bd847..4fc6b32 100644
>>> --- a/src/libvirt_private.syms
>>> +++ b/src/libvirt_private.syms
>>> @@ -143,7 +143,6 @@ virDomainDiskDeviceTypeToString;
>>> virDomainDiskErrorPolicyTypeFromString;
>>> virDomainDiskErrorPolicyTypeToString;
>>> virDomainDiskFindByBusAndDst;
>>> -virDomainDiskFindControllerModel;
>>> virDomainDiskGeometryTransTypeFromString;
>>> virDomainDiskGeometryTransTypeToString;
>>> virDomainDiskHostDefFree;
>>> @@ -213,6 +212,7 @@ virDomainHubTypeFromString;
>>> virDomainHubTypeToString;
>>> virDomainHypervTypeFromString;
>>> virDomainHypervTypeToString;
>>> +virDomainInfoFindControllerModel;
>>> virDomainInputDefFree;
>>> virDomainIoEventFdTypeFromString;
>>> virDomainIoEventFdTypeToString;
>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>>> index 201fac1..5eb9999 100644
>>> --- a/src/qemu/qemu_command.c
>>> +++ b/src/qemu/qemu_command.c
>>> @@ -549,7 +549,7 @@ qemuAssignDeviceDiskAliasCustom(virDomainDefPtr def,
>>> if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) {
>>> if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) {
>>> controllerModel =
>>> - virDomainDiskFindControllerModel(def, disk,
>>> + virDomainInfoFindControllerModel(def,&disk->info,
>>> VIR_DOMAIN_CONTROLLER_TYPE_SCSI);
>>>
>>> if ((qemuSetScsiControllerModel(def, qemuCaps,&controllerModel))< 0)
>>> @@ -2699,7 +2699,7 @@ qemuBuildDriveDevStr(virDomainDefPtr def,
>>> }
>>>
>>> controllerModel =
>>> - virDomainDiskFindControllerModel(def, disk,
>>> + virDomainInfoFindControllerModel(def,&disk->info,
>>> VIR_DOMAIN_CONTROLLER_TYPE_SCSI);
>>> if ((qemuSetScsiControllerModel(def, qemuCaps,&controllerModel))< 0)
>>> goto error;
>>
>> You need new tests for the new XML.
>>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list