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

[Thanos Makatos <thanos.makatos@xxxxxxxxxx>]

> -----Original Message-----
> From: Ian Campbell
> Sent: 26 October 2012 12:46
> To: Thanos Makatos
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH 16 of 16 RFC] blktap3: Implement
> libxl__blktap_devpath and libxl__device_destroy_tapdisk
> 
> 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?

Yes.

> 
> >  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.

I've added this so I can build libxl from within tools/libxl without worrying 
about building tools/blktap3, it simplifies development, doesn't it?

> 
> >     $(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?

I don't think so, I'll have a look. This could be a problem if two guest VMs 
are created at the same time and they both use the same VDI, obviously a corner 
case. Should we protect against that or we're getting paranoid?

> 
> >  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.

Ok.

> 
> Ian.

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