[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH RFC 1/5] conf: Introduce readonly to hostdev and change helper function



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.

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.
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]