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