Re: [libvirt] [PATCH] libxl: add support for mounts in HVM guests

On 12/01/2015 08:04 AM, Cédric Bosdonnat wrote: > Xen HVM guests are running a QEMU, libxl allows passing QEMU parameters > in the d_config.b_info.extra list. Take advantage of that to implement > support for 9pfs mounts. For this to work, xen's qemu needs to be built > with virtfs enabled, or an upstream qemu needs to be used. What if the qemu doesn't have virtfs enabled? Does it fail to start? Is a reasonable error returned to the user or available in /var/log/libvirt/libxl/libxl-driver.log or /var/log/xen/qemu-dm-<vmname>.log? The reason for a qemu start failure can usually be found in /var/log/xen/qemu-dm-<vmname>.log. If a reasonable error is reported there, I think we are fine.

> --- > Note that two functions have been picked from the qemu driver's code. > > src/libxl/libxl_conf.c | 174 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 174 insertions(+) > > diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c > index 4eed5ca..4437e8f 100644 > --- a/src/libxl/libxl_conf.c > +++ b/src/libxl/libxl_conf.c > @@ -55,6 +55,7 @@ VIR_LOG_INIT("libxl.libxl_conf"); > /* see xen-unstable.hg/xen/include/asm-x86/cpufeature.h */ > #define LIBXL_X86_FEATURE_PAE_MASK 0x40 > > +#define LIBXL_FSDEV_HOST_PREFIX "fsdev-" > > struct guest_arch { > virArch arch; > @@ -84,6 +85,15 @@ static int libxlConfigOnceInit(void) > > VIR_ONCE_GLOBAL_INIT(libxlConfig) > > +VIR_ENUM_DECL(libxlDomainFSDriver) > +VIR_ENUM_IMPL(libxlDomainFSDriver, VIR_DOMAIN_FS_DRIVER_TYPE_LAST, > + "local", > + "local", > + "handle", > + NULL, > + NULL, > + NULL); > + > static void > libxlDriverConfigDispose(void *obj) > { > @@ -1854,6 +1864,167 @@ libxlMakeCapabilities(libxl_ctx *ctx) > return NULL; > } > > +static char * > +libxlBuildQemuFSStr(virDomainFSDefPtr fs) > +{ > + virBuffer opt = VIR_BUFFER_INITIALIZER; > + const char *driver = libxlDomainFSDriverTypeToString(fs->fsdriver); > + const char *wrpolicy = virDomainFSWrpolicyTypeToString(fs->wrpolicy); > + > + if (fs->type != VIR_DOMAIN_FS_TYPE_MOUNT) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("only supports mount filesystem type")); > + goto error; > + } > + > + if (!driver) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("Filesystem driver type not supported")); > + goto error; > + } > + virBufferAdd(&opt, driver, -1); > + > + if (fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_PATH || > + fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_DEFAULT) { > + if (fs->accessmode == VIR_DOMAIN_FS_ACCESSMODE_MAPPED) { > + virBufferAddLit(&opt, ",security_model=mapped"); > + } else if (fs->accessmode == VIR_DOMAIN_FS_ACCESSMODE_PASSTHROUGH) { > + virBufferAddLit(&opt, ",security_model=passthrough"); > + } else if (fs->accessmode == VIR_DOMAIN_FS_ACCESSMODE_SQUASH) { > + virBufferAddLit(&opt, ",security_model=none"); > + } > + } else { > + /* For other fs drivers, default(passthru) should always > + * be supported */ > + if (fs->accessmode != VIR_DOMAIN_FS_ACCESSMODE_PASSTHROUGH) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("only supports passthrough accessmode")); > + goto error; > + } > + } > + > + if (fs->wrpolicy) > + virBufferAsprintf(&opt, ",writeout=%s", wrpolicy); > + > + virBufferAsprintf(&opt, ",id=%s%s", LIBXL_FSDEV_HOST_PREFIX, fs->info.alias); > + virBufferAsprintf(&opt, ",path=%s", fs->src); > + > + if (fs->readonly) > + virBufferAddLit(&opt, ",readonly"); > + > + if (virBufferCheckError(&opt) < 0) > + goto error; > + > + return virBufferContentAndReset(&opt); > + > + error: > + virBufferFreeAndReset(&opt); > + return NULL; > +} > + > + > +static char * > +libxlBuildQemuFSDevStr(virDomainFSDefPtr fs) > +{ > + virBuffer opt = VIR_BUFFER_INITIALIZER; > + > + if (fs->type != VIR_DOMAIN_FS_TYPE_MOUNT) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("can only passthrough directories")); > + goto error; > + } > + > + if (fs->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) > + virBufferAddLit(&opt, "virtio-9p-ccw"); > + else > + virBufferAddLit(&opt, "virtio-9p-pci"); > + > + virBufferAsprintf(&opt, ",id=%s", fs->info.alias); > + virBufferAsprintf(&opt, ",fsdev=%s%s", LIBXL_FSDEV_HOST_PREFIX, fs->info.alias); > + virBufferAsprintf(&opt, ",mount_tag=%s", fs->dst); > + > + if (virBufferCheckError(&opt) < 0) > + goto error; > + > + return virBufferContentAndReset(&opt); > + > + error: > + virBufferFreeAndReset(&opt); > + return NULL; > +} I suppose the copy and paste is fine, but would like to hear what others have to say. We could revisit the idea of a 'qemu command line builder' utility lib if we find ourselves copying more and more of the code.

> + > +static int > +libxlMakeMountList(virDomainDefPtr def, > + libxl_domain_config *d_config) > +{ > + int ret = -1; > + size_t i; > + size_t nextra = 0; > + char *fsstr = NULL; > + char *fsdevstr = NULL; > + char *fsdevargstr = NULL; > + char *deviceargstr = NULL; > + > + if (def->nfss > 0 && def->os.type != VIR_DOMAIN_OSTYPE_HVM) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("only HVM guests supports mounts")); > + return ret; > + } > + > + /* Make sure we have an allocated array, not ending with NULL */ > + if (!d_config->b_info.extra) { > + if (VIR_ALLOC(d_config->b_info.extra) < 0) > + return ret; > + nextra = 0; > + } else { > + nextra = virStringListLength(d_config->b_info.extra) + 1; > + if (VIR_DELETE_ELEMENT(d_config->b_info.extra, nextra, nextra) < 0) > + return ret; > + } > + > + /* Add the QEMU options to d_config->b_info->extra > + * -fsdev local,id=virtio9p,path=%s,security_model=none > + * -device virtio-9p-pci,fsdev=virtio9p,mount_tag=%s > + */ > + for (i = 0; i < def->nfss; i++) { > + virDomainFSDefPtr fs = def->fss[i]; > + > + /* Assign an alias */ > + if (virAsprintf(&fs->info.alias, "fs%zu", i) < 0) > + goto cleanup; > + > + if (!(fsstr = libxlBuildQemuFSStr(fs))) > + goto cleanup; > + if (!(fsdevstr = libxlBuildQemuFSDevStr(fs))) > + goto cleanup; > + > + if (VIR_STRDUP(fsdevargstr, "-fsdev") < 0 || > + VIR_STRDUP(deviceargstr, "-device") < 0) > + goto cleanup; > + > + > + /* Append to the extras before terminating NULL */ > + if (VIR_APPEND_ELEMENT(d_config->b_info.extra, nextra, fsdevargstr) < 0 || > + VIR_APPEND_ELEMENT(d_config->b_info.extra, nextra, fsstr) < 0 || > + VIR_APPEND_ELEMENT(d_config->b_info.extra, nextra, deviceargstr) < 0 || > + VIR_APPEND_ELEMENT(d_config->b_info.extra, nextra, fsdevstr) < 0) > + goto cleanup; > + } > + ret = 0; > + > + /* Add the terminating NULL item */ > + if (VIR_RESIZE_N(d_config->b_info.extra, nextra, nextra, 1) < 0) > + goto cleanup; > + d_config->b_info.extra[nextra++] = NULL; > + > + cleanup: > + VIR_FREE(fsstr); > + VIR_FREE(fsdevstr); > + VIR_FREE(fsdevargstr); > + VIR_FREE(deviceargstr); > + return ret; > +} > + > int > libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, > virDomainDefPtr def, > @@ -1883,6 +2054,9 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, > if (libxlMakePCIList(def, d_config) < 0) > return -1; > > + if (libxlMakeMountList(def, d_config) < 0) > + return -1; > + Otherwise this looks good! Weak ACK based on answer to qemu not supporting virtfs, and another opinion on the copied code. Regards, Jim