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

[Ian Jackson <Ian.Jackson@eu.citrix.com>]

Olaf Hering writes ("[PATCH v10 4/5] libxl: add support for vscsi"):
> Port pvscsi support from xend to libxl:
> 
>  vscsi=['pdev,vdev{,options}']
>  xl scsi-attach
>  xl scsi-detach
>  xl scsi-list

Thanks for this contribution.  I have tried to review it, although I
don't fully understand the underlying vscsi subsystem.  The public API
looks reasonably good.


> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 59b183c..b670997 100644
> --- a/tools/libxl/libxl_types.idl
...
> +libxl_vscsiinfo = Struct("vscsiinfo", [
> +    ("backend", string),
> +    ("backend_id", uint32),
> +    ("frontend", string),
> +    ("frontend_id", uint32),
> +    ("devid", libxl_devid),
> +    ("pdev", libxl_vscsi_pdev),
> +    ("vdev", libxl_vscsi_hctl),
> +    ("idx", libxl_devid),
> +    ("vscsidev_id", libxl_devid),
> +    ("scsi_raw_cmds", bool),
> +    ("vscsictrl_state", integer),
> +    ("vscsidev_state", integer),
> +    ("evtch", integer),
> +    ("rref", integer),

I don't think the event channel and ring ref belong here.  (That they
are present in some other devices is a mistake.)


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

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

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.



Thanks,
Ian.

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