Re: [Xen-devel] [PATCH v10 4/5] libxl: add support for vscsi

[Olaf Hering <olaf@xxxxxxxxx>]

On Fri, Apr 01, Ian Jackson wrote:

> > +static int xlu__vscsi_parse_dev(XLU_Config *cfg, char *pdev, 
> > libxl_vscsi_hctl *hctl)
> > +{
> > +    struct stat dentry;
> > +    char *sysfs = NULL;
> > +    const char *type;
> > +    int rc, found = 0;
> > +    DIR *dirp;
> > +    struct dirent *de;
> ...
> > +    /* /sys/dev/type/major:minor symlink added in 2.6.27 */
> > +    if (asprintf(&sysfs, "/sys/dev/%s/%u:%u/device/scsi_device", type,
> > +                 major(dentry.st_rdev), minor(dentry.st_rdev)) < 0) {
> > +        sysfs = NULL;
> > +        rc = ERROR_NOMEM;
> > +        goto out;
> > +    }
> 
> I'm not sure that this sysfs parsing ought to be in xl rather than
> libxl.  Also, this is Linux-specific code.  So it needs to be made
> conditional somehow.

I think this depends on how libvirt is supposed to interact with libxl
here. Right now libvirt is rather dumb, it supports just the
host:channel:target:lun notation for a backend. My patch for it (which I
have to rebase now) creates a string "pdev,vdev" and calls
xlu_vscsi_get_host.  I will rebase the libvirt patch and resend it as
well.

> It seems to me that the contents of xlu__vscsi_target should be much
> closer to vscsi_pdev_type (unless I have misunderstood).

Well, if its important where that struct is supposed to be in the file,
I can move it down.

> Perhaps the libxl_types.idl API needs to change.  In general, the
> libxl API ought to be close enough in semantics to the xl config API
> that the correspondence is obvious.  I don't think that's the case
> here.

I'm not sure what this paragraph means.


Olaf

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