On Tue, 2015-12-01 at 21:59 -0700, Jim Fehlig wrote:
> 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.
The domain fails to start in that case, but the error isn't
understandable (even a developer like me couldn't figure out what it
was).
I'll see if I can add a test for that to fail earlier with a meaningful
error message.
> > ---
> > 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.
I was hesitating doing that too... Daniel pushed me into that. However I
removed the qemuCaps-related pieces from the qemu driver original code,
not sure if we can factorize this.
--
Cedric
> > +
> > +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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list