Re: [Xen-devel] [PATCH 4 of 4] libxl: Use libblktapctl.so

[Daniel Stodden <daniel.stodden@xxxxxxxxxx>]

On Thu, 2010-06-10 at 12:25 -0400, Stefano Stabellini wrote:
> > -static char *get_blktap2_device(struct libxl_ctx *ctx, char *name, char 
> > *type)
> > +static char *make_blktap2_device(struct libxl_ctx *ctx,
> > +                            const char *name, const char *type)
> >  {
> > -    char buf[1024];
> > -    char *p;
> > -    int devnum;
> > -    FILE *f = fopen("/sys/class/blktap2/devices", "r");
> > -
> > -    
> > -    while (!feof(f)) {
> > -        if (fscanf(f, "%d %s", &devnum, buf) != 2)
> > -            continue;
> > -        p = strchr(buf, ':');
> > -        if (p == NULL)
> > -            continue;
> > -        p++;
> > -        if (!strcmp(p, name) && !strncmp(buf, type, 3)) {
> > -            fclose(f);
> > -            return libxl_sprintf(ctx, "/dev/xen/blktap-2/tapdev%d", 
> > devnum);
> > -        }
> > -    }
> > -    fclose(f);
> > -    return NULL;
> > +    char *params, *devname;
> > +    int err;
> > +    params = libxl_sprintf(ctx, "%s:%s", type, name);
> > +    devname = NULL;
> > +    err = tap_ctl_create(params, &devname);
> > +    free(params);
> > +    return err ? NULL : devname;
> >  }
> > 
> 
> you shouldn't free pointers returned by libxl internal functions,
> because libxl will take care of free'ing them.

I'll send an update. Sorry for that. Please don't hesitate to submit
corrections right away if you spot more issues.
 
>                  dev = buf;
> > -                }
> > +        case PHYSTYPE_AIO: case PHYSTYPE_QCOW: case PHYSTYPE_QCOW2: case 
> > PHYSTYPE_VHD: {
> > +            const char *msg;
> > +            if (!tap_ctl_check(&msg)) {
> > +                const char *type = 
> > device_disk_string_of_phystype(disk->phystype);
> > +                char *dev;
> > +                dev = get_blktap2_device(ctx, disk->physpath, type);
> > +                if (!dev)
> > +                    dev = make_blktap2_device(ctx, disk->physpath, type);
> > +                if (!dev)
> > +                    return -1;
> >                  flexarray_set(back, boffset++, "tapdisk-params");
> >                  flexarray_set(back, boffset++, libxl_sprintf(ctx, "%s:%s", 
> > device_disk_string_of_phystype(disk->phystype), disk->physpath));
> >                  flexarray_set(back, boffset++, "params");
> > 
> 
> you are calling make_blktap2_device only if(!dev), are you sure it is
> correct? get_blktap2_device returns a pointer on success...

I don't quite manage to follow this comment. The behavior was meant to
match the original version of that code. It still looks like it does. Is
that what you question?

Unless you're worried about null pointer handling with massively broken
compilers? Did you ever manage to find one? I didn't %-)

But again, if you're not happy with it, please just go ahead and make it
suit your coding preferences. I got zero stakes in that code. 

Btw, someone probably should also fix the device teardown path as well,
it seems to be consistently leaking device nodes. I guess enumerating
backend physical-device nodes to count vbds would probably do for a
'light' control layer.

Thanks for the input.

Daniel



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