Re: [Xen-devel] [PATCH 16 of 16 RFC] blktap3: Implement libxl__blktap_devpath and libxl__device_destroy_tapdisk

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2012-10-24 at 18:02 +0100, Thanos Makatos wrote:
> Provide implementation for the libxl__blktap_devpath and
> libxl__device_destroy_tapdisk functions: the former spawns the tapdisk 
> process,
> the latter destroys it. Both of these functions use the blktap_find function,
> a function that lists all running tapdisks and looks for one serving a 
> specific
> file. Finally, link libxl with the blktap3 control library.
> 
> diff -r b12c1bb767d3 -r 0f87cc018fb6 tools/libxl/Makefile
> --- a/tools/libxl/Makefile    Wed Oct 24 17:28:12 2012 +0100
> +++ b/tools/libxl/Makefile    Wed Oct 24 17:42:44 2012 +0100
> @@ -20,7 +20,7 @@ LIBUUID_LIBS += -luuid
>  endif
>  
>  LIBXL_LIBS =
> -LIBXL_LIBS = $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) 
> $(LDLIBS_libxenstore) $(PTYFUNCS_LIBS) $(LIBUUID_LIBS)
> +LIBXL_LIBS = $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) 
> $(LDLIBS_libxenstore) $(PTYFUNCS_LIBS) $(LIBUUID_LIBS) $(LDLIBS_libblktapctl)

Is this adding back the thing I commented on earlier?

>  CFLAGS_LIBXL += $(CFLAGS_libxenctrl)
>  CFLAGS_LIBXL += $(CFLAGS_libxenguest)
> @@ -29,7 +29,9 @@ CFLAGS_LIBXL += $(CFLAGS_libblktapctl)
>  CFLAGS_LIBXL += -Wshadow
>  
>  CFLAGS += $(PTHREAD_CFLAGS)
> -LDFLAGS += $(PTHREAD_LDFLAGS)
> +override LDFLAGS += \
> +     $(PTHREAD_LDFLAGS) \
> +     -L $(XEN_BLKTAP3)/control

Please (defined and) use LDLIBS_libblktapctl. Also this change to use
override looks very dubious to me -- what does it do?

>  LIBXL_LIBS += $(PTHREAD_LIBS)
>  
>  LIBXLU_LIBS =
> @@ -172,6 +174,7 @@ libxenlight.so.$(MAJOR): libxenlight.so.
>       ln -sf $< $@
>  
>  libxenlight.so.$(MAJOR).$(MINOR): $(LIBXL_OBJS)
> +     make -C $(XEN_BLKTAP3)

This shouldn't be needed if the tools/Makefile has things ordered
correctly. It is reasonable for tools/libxl to assume that its
prerequisites are already built. People who build with make -C
tools/libxl need to take care of this themself.

>       $(CC) $(LDFLAGS) -Wl,$(SONAME_LDFLAG) -Wl,libxenlight.so.$(MAJOR) 
> $(SHLIB_LDFLAGS) -o $@ $^ $(LIBXL_LIBS) $(APPEND_LDFLAGS)
>  
>  libxenlight.a: $(LIBXL_OBJS)
> diff -r b12c1bb767d3 -r 0f87cc018fb6 tools/libxl/libxl_blktap3.c
> --- a/tools/libxl/libxl_blktap3.c     Wed Oct 24 17:28:12 2012 +0100
> +++ b/tools/libxl/libxl_blktap3.c     Wed Oct 24 17:42:44 2012 +0100
>  
> -int libxl__device_destroy_tapdisk(libxl__gc *gc, const char *be_path) {
> -    return -ENOSYS;
> +/**
> + * Creates a tapdisk for the specified path.
> + *
> + * TODO document parameters
> + *
> + * @param gc
> + * @param disk
> + * @param format
> + *
> + * @returns 0 on success, an error code otherwise
> + */
> +int libxl__blktap_devpath(libxl__gc *gc, const char *disk,
> +        libxl_disk_format format)
> +{
> +    const char *type = NULL;
> +    char *params = NULL;
> +    struct tap_list tap;
> +    int err = 0;
> +
> +    type = libxl__device_disk_string_of_format(format);
> +
> +    /*
> +     * Ensure that no other tapdisk is serving this path.
> +     * XXX Does libxl protect us against race conditions?

Not AFAIK. Does tapdisk not open the file O_EXCL when necessary?

>  What if somebody
> +     * manually attaches a tapdisk to this path?
> +     */
> +    if (!(err = blktap_find(gc, type, disk, &tap))) {
> +        LOG(DEBUG, "tapdisk %d already serving %s\n", tap.pid, disk);
> +        return 0;
> +    }
> +
> +    LOG(DEBUG, "tapdisk not found\n");
> +
> +    /*
> +     * TODO Should we worry about return codes other than ENOENT?
> +     */
> +
> +    params = libxl__sprintf(gc, "%s:%s", type, disk);
> +    if (!(err = tap_ctl_create(params, 0, -1, NULL))) {
> +        LOG(DEBUG, "created tapdisk\n");        
> +        return 0;
> +    }
> +
> +    LOG(ERROR, "error creating tapdisk: %s\n", strerror(err));

You can use LOGE or LOGEV here to get the errno type thing printed
automatically in a consistent way.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel