Xen devel - Aug 2010 - [PATCH] xl: Make blktap support optional

Make blktap support optional.
Enable it by default on Linux, disable it on non-Linux.

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>

-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Christoph Egger writes ("[Xen-devel] [PATCH] xl: Make blktap support
optional"):
> Make blktap support optional.
> Enable it by default on Linux, disable it on non-Linux.
This patch still contains all the mixed up code motion, formatting and
ifdefs that I criticised in previous instances.

Nacked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Fri, 2010-08-13 at 15:06 +0100, Christoph Egger
wrote:
> diff -r 2760576b0d7c -r 9996d4f06e70 tools/libxl/libxl_blktap2.c
> --- /dev/null
> +++ b/tools/libxl/libxl_blktap2.c
> @@ -0,0 +1,51 @@
> +/*
> + * Copyright (C) 2010      Advanced Micro Devices
> + * Author Christoph Egger <Christoph.Egger@amd.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as
published
> + * by the Free Software Foundation; version 2.1 only.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU Lesser General Public License for more details.
> + */
> +
> +#include "libxl.h"
> +#include "libxl_osdeps.h"
> +#include "libxl_internal.h"
> +
> +#include "tap-ctl.h"
> +
> +int libxl_blktap_enabled(libxl_ctx *ctx)
> +{
> +    const char *msg;
> +    return !tap_ctl_check(&msg);
> +}
> +
> +const char *libxl_blktap_devpath(libxl_ctx *ctx,
> +                                 const char *disk,
> +                                 libxl_disk_phystype phystype)
> +{
> +    const char *type;
> +    char *params, *devname;
> +    int minor, err;
> +
> +    type = device_disk_string_of_phystype(phystype);
> +    minor = tap_ctl_find_minor(type, disk);
> +    if (minor >= 0) {
> +        devname = libxl_sprintf(ctx,
"/dev/xen/blktap-2/tapdev%d", minor);
> +        if (devname)
> +            return devname;
> +    }
This actually breaks the build on linux. It''s because this part of the
patch needs a re-base since libxl_sprintf() etc. no longer takes a ctx
argument. You will need to initialise a gc to do the libxl_sprintf() and
then strdup() the result for the caller. The parts where you patched out
of libxl.c had equivalent changes.
> +    params = libxl_sprintf(ctx, "%s:%s", type, disk);
> +    err = tap_ctl_create(params, &devname);
> +    if (!err) {
> +        libxl_ptr_add(ctx, devname);
> +        return devname;
> +    }
libxl_ptr_add will no longer be necessary when re-based as above.
> +
> +    return NULL;
> +} 
Other than that, which changed under your feet, I think the patch is
fine.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Mon, 16 Aug 2010, Gianni Tedesco wrote:
> On Fri, 2010-08-13 at 15:06 +0100, Christoph Egger wrote:
> > diff -r 2760576b0d7c -r 9996d4f06e70 tools/libxl/libxl_blktap2.c
> > --- /dev/null
> > +++ b/tools/libxl/libxl_blktap2.c
> > @@ -0,0 +1,51 @@
> > +/*
> > + * Copyright (C) 2010      Advanced Micro Devices
> > + * Author Christoph Egger <Christoph.Egger@amd.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
modify
> > + * it under the terms of the GNU Lesser General Public License as
published
> > + * by the Free Software Foundation; version 2.1 only.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU Lesser General Public License for more details.
> > + */
> > +
> > +#include "libxl.h"
> > +#include "libxl_osdeps.h"
> > +#include "libxl_internal.h"
> > +
> > +#include "tap-ctl.h"
> > +
> > +int libxl_blktap_enabled(libxl_ctx *ctx)
> > +{
> > +    const char *msg;
> > +    return !tap_ctl_check(&msg);
> > +}
> > +
> > +const char *libxl_blktap_devpath(libxl_ctx *ctx,
> > +                                 const char *disk,
> > +                                 libxl_disk_phystype phystype)
> > +{
> > +    const char *type;
> > +    char *params, *devname;
> > +    int minor, err;
> > +
> > +    type = device_disk_string_of_phystype(phystype);
> > +    minor = tap_ctl_find_minor(type, disk);
> > +    if (minor >= 0) {
> > +        devname = libxl_sprintf(ctx,
"/dev/xen/blktap-2/tapdev%d", minor);
> > +        if (devname)
> > +            return devname;
> > +    }
> 
> This actually breaks the build on linux. It''s because this part of
the
> patch needs a re-base since libxl_sprintf() etc. no longer takes a ctx
> argument. You will need to initialise a gc to do the libxl_sprintf() and
> then strdup() the result for the caller. The parts where you patched out
> of libxl.c had equivalent changes.
> 
> > +    params = libxl_sprintf(ctx, "%s:%s", type, disk);
> > +    err = tap_ctl_create(params, &devname);
> > +    if (!err) {
> > +        libxl_ptr_add(ctx, devname);
> > +        return devname;
> > +    }
> 
> libxl_ptr_add will no longer be necessary when re-based as above.
> 
> > +
> > +    return NULL;
> > +} 
> 
> Other than that, which changed under your feet, I think the patch is
> fine.
I''ll rebase, fix and apply this patch myself today.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Mon, 16 Aug 2010, Gianni Tedesco wrote:
> On Fri, 2010-08-13 at 15:06 +0100, Christoph Egger wrote:
> > diff -r 2760576b0d7c -r 9996d4f06e70 tools/libxl/libxl_blktap2.c
> > --- /dev/null
> > +++ b/tools/libxl/libxl_blktap2.c
> > @@ -0,0 +1,51 @@
> > +/*
> > + * Copyright (C) 2010      Advanced Micro Devices
> > + * Author Christoph Egger <Christoph.Egger@amd.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
modify
> > + * it under the terms of the GNU Lesser General Public License as
published
> > + * by the Free Software Foundation; version 2.1 only.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU Lesser General Public License for more details.
> > + */
> > +
> > +#include "libxl.h"
> > +#include "libxl_osdeps.h"
> > +#include "libxl_internal.h"
> > +
> > +#include "tap-ctl.h"
> > +
> > +int libxl_blktap_enabled(libxl_ctx *ctx)
> > +{
> > +    const char *msg;
> > +    return !tap_ctl_check(&msg);
> > +}
> > +
> > +const char *libxl_blktap_devpath(libxl_ctx *ctx,
> > +                                 const char *disk,
> > +                                 libxl_disk_phystype phystype)
> > +{
> > +    const char *type;
> > +    char *params, *devname;
> > +    int minor, err;
> > +
> > +    type = device_disk_string_of_phystype(phystype);
> > +    minor = tap_ctl_find_minor(type, disk);
> > +    if (minor >= 0) {
> > +        devname = libxl_sprintf(ctx,
"/dev/xen/blktap-2/tapdev%d", minor);
> > +        if (devname)
> > +            return devname;
> > +    }
> 
> This actually breaks the build on linux. It''s because this part of
the
> patch needs a re-base since libxl_sprintf() etc. no longer takes a ctx
> argument. You will need to initialise a gc to do the libxl_sprintf() and
> then strdup() the result for the caller. The parts where you patched out
> of libxl.c had equivalent changes.
> 
considering that these are internal function, I''ll pass a libxl_gc
instead of a libxl_ctx.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel