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

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

 



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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]