[libvirt] [PATCH] Added new attribute exportfs_type to filesystem element

This patch introduces a new attribute export_fs to the filesystem element which specifies the type of export. Currently only 'local' type of exported filesystem is supported. More types like NFS, clusterFS, etc. can be added later as required. Note: This patch is based on the following two patches: 1) Daniel's patch to support 9pfs: https://www.redhat.com/archives/libvir-list/2010-September/msg00358.html 2) Another related patch to support 'security_model' attribute: https://www.redhat.com/archives/libvir-list/2010-September/msg00435.html Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com> --- docs/schemas/domain.rng | 5 +++++ src/conf/domain_conf.c | 24 ++++++++++++++++++++++-- src/conf/domain_conf.h | 9 +++++++++ src/qemu/qemu_conf.c | 3 ++- 4 files changed, 38 insertions(+), 3 deletions(-) diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index 43a292d..5b7563a 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -768,6 +768,11 @@ <value>none</value> </choice> </attribute> + <attribute name="exportfs_type"> + <choice> + <value>local</value> + </choice> + </attribute> </optional> </element> </define> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a9881d1..6d728a7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -166,6 +166,8 @@ VIR_ENUM_IMPL(virDomainFSSecurityModel, VIR_DOMAIN_FS_SECURITY_LAST, "mapped", "none") +VIR_ENUM_IMPL(virDomainFSExportType, VIR_DOMAIN_FS_EXPORT_TYPE_LAST, + "local") VIR_ENUM_IMPL(virDomainNet, VIR_DOMAIN_NET_TYPE_LAST, "user", @@ -1851,6 +1853,7 @@ virDomainFSDefParseXML(xmlNodePtr node, virDomainFSDefPtr def; xmlNodePtr cur; char *type = NULL; + char *exportfs_type = NULL; char *source = NULL; char *target = NULL; char *security_model; @@ -1871,6 +1874,17 @@ virDomainFSDefParseXML(xmlNodePtr node, def->type = VIR_DOMAIN_FS_TYPE_MOUNT; } + exportfs_type = virXMLPropString(node, "exportfs_type"); + if (exportfs_type) { + if ((def->exportfs_type = virDomainFSExportTypeTypeFromString(exportfs_type)) < 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown exportfs type '%s'"), exportfs_type); + goto error; + } + } else { + def->exportfs_type = VIR_DOMAIN_FS_EXPORT_TYPE_LOCAL; + } + security_model = virXMLPropString(node, "security_model"); if (security_model) { if ((def->security_model = virDomainFSSecurityModelTypeFromString(security_model)) < 0) { @@ -5621,6 +5635,7 @@ virDomainFSDefFormat(virBufferPtr buf, { const char *type = virDomainFSTypeToString(def->type); const char *sec_model = virDomainFSSecurityModelTypeToString(def->security_model); + const char *expfs_type = virDomainFSExportTypeTypeToString(def->exportfs_type); if (!type) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, @@ -5628,6 +5643,11 @@ virDomainFSDefFormat(virBufferPtr buf, return -1; } + if (!expfs_type) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected exportfs type %d"), def->type); + } + if (!sec_model) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected security model %d"), def->security_model); @@ -5636,8 +5656,8 @@ virDomainFSDefFormat(virBufferPtr buf, virBufferVSprintf(buf, - " <filesystem type='%s' security_model='%s'>\n", - type, sec_model); + " <filesystem type='%s' exportfs_type='%s' security_model='%s'>\n", + type, expfs_type, sec_model); if (def->src) { switch (def->type) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 6adf027..956cac0 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -245,10 +245,18 @@ enum virDomainFSSecurityModel { VIR_DOMAIN_FS_SECURITY_LAST }; +/* Export Filesystem Type */ +enum virDomainFSExportType { + VIR_DOMAIN_FS_EXPORT_TYPE_LOCAL, + + VIR_DOMAIN_FS_EXPORT_TYPE_LAST +}; + typedef struct _virDomainFSDef virDomainFSDef; typedef virDomainFSDef *virDomainFSDefPtr; struct _virDomainFSDef { int type; + int exportfs_type; int security_model; char *src; char *dst; @@ -1177,6 +1185,7 @@ VIR_ENUM_DECL(virDomainDiskErrorPolicy) VIR_ENUM_DECL(virDomainController) VIR_ENUM_DECL(virDomainControllerModel) VIR_ENUM_DECL(virDomainFS) +VIR_ENUM_DECL(virDomainFSExportType) VIR_ENUM_DECL(virDomainNet) VIR_ENUM_DECL(virDomainChrDevice) VIR_ENUM_DECL(virDomainChrChannelTarget) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 6b96d2f..458beab 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -2788,7 +2788,8 @@ char *qemuBuildFSStr(virDomainFSDefPtr fs, goto error; } - virBufferAddLit(&opt, "local"); + if (fs->exportfs_type == VIR_DOMAIN_FS_EXPORT_TYPE_LOCAL) + virBufferAddLit(&opt, "local"); if (fs->security_model == VIR_DOMAIN_FS_SECURITY_PASSTHROUGH) virBufferAddLit(&opt, ",security_model=passthrough"); else if (fs->security_model == VIR_DOMAIN_FS_SECURITY_MAPPED) -- 1.7.1.1

On Thu, Sep 30, 2010 at 10:30:10PM +0530, Harsh Prateek Bora wrote:
This patch introduces a new attribute export_fs to the filesystem element which specifies the type of export. Currently only 'local' type of exported filesystem is supported. More types like NFS, clusterFS, etc. can be added later as required.
Note: This patch is based on the following two patches: 1) Daniel's patch to support 9pfs: https://www.redhat.com/archives/libvir-list/2010-September/msg00358.html 2) Another related patch to support 'security_model' attribute: https://www.redhat.com/archives/libvir-list/2010-September/msg00435.html
Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>
Okay, I don't understand what's the point of adding that attribute with only one possible value and systematically generated. I think it's better to propose this when there is an actual use case for the attribute, then it will be easier to say if this is the right construct to add or not. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Wed, Oct 06, 2010 at 06:22:29PM +0200, Daniel Veillard wrote:
On Thu, Sep 30, 2010 at 10:30:10PM +0530, Harsh Prateek Bora wrote:
This patch introduces a new attribute export_fs to the filesystem element which specifies the type of export. Currently only 'local' type of exported filesystem is supported. More types like NFS, clusterFS, etc. can be added later as required.
Note: This patch is based on the following two patches: 1) Daniel's patch to support 9pfs: https://www.redhat.com/archives/libvir-list/2010-September/msg00358.html 2) Another related patch to support 'security_model' attribute: https://www.redhat.com/archives/libvir-list/2010-September/msg00435.html
Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>
Okay, I don't understand what's the point of adding that attribute with only one possible value and systematically generated. I think it's better to propose this when there is an actual use case for the attribute, then it will be easier to say if this is the right construct to add or not.
I agree and in fact think this extra attribute is almost certainly the wrong approach. The existing <filesystem type='....'> attribute should be sufficient for our needs. When QEMU supports FS backends which are not 'local', then we will likely add extra values for type='...' to cope with them. So lets just wait until QEMU actually supports some non-local modes. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Hi Daniel, Makes sense, agreed. However, did you get a chance to review the security_model patch? https://www.redhat.com/archives/libvir-list/2010-September/msg00435.html On 10/06/2010 09:59 PM, Daniel P. Berrange wrote:
On Wed, Oct 06, 2010 at 06:22:29PM +0200, Daniel Veillard wrote:
On Thu, Sep 30, 2010 at 10:30:10PM +0530, Harsh Prateek Bora wrote:
This patch introduces a new attribute export_fs to the filesystem element which specifies the type of export. Currently only 'local' type of exported filesystem is supported. More types like NFS, clusterFS, etc. can be added later as required.
Note: This patch is based on the following two patches: 1) Daniel's patch to support 9pfs: https://www.redhat.com/archives/libvir-list/2010-September/msg00358.html 2) Another related patch to support 'security_model' attribute: https://www.redhat.com/archives/libvir-list/2010-September/msg00435.html
Signed-off-by: Harsh Prateek Bora<harsh@linux.vnet.ibm.com>
Okay, I don't understand what's the point of adding that attribute with only one possible value and systematically generated. I think it's better to propose this when there is an actual use case for the attribute, then it will be easier to say if this is the right construct to add or not.
I agree and in fact think this extra attribute is almost certainly the wrong approach. The existing<filesystem type='....'> attribute should be sufficient for our needs. When QEMU supports FS backends which are not 'local', then we will likely add extra values for type='...' to cope with them. So lets just wait until QEMU actually supports some non-local modes.
Regards, Daniel
participants (4)
-
Daniel P. Berrange
-
Daniel Veillard
-
Harsh Bora
-
Harsh Prateek Bora