Re: [Xen-devel] [PATCH v11 1/2] libxl: add support for vscsi

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Fri, Apr 08, 2016 at 07:01:30AM +0000, Olaf Hering wrote:
> Port pvscsi support from xend to libxl:
> 
>  vscsi=['pdev,vdev{,options}']
>  xl scsi-attach
>  xl scsi-detach
>  xl scsi-list
> 
> Signed-off-by: Olaf Hering <olaf@xxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>

This patch looks quite close now.

I have one comment below, which needs to be addressed properly.

Also there are some higher level questions that are not settled in the
last iteration, I will copy and paste it here:

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

I guess the answer is that we should leave the parsing inside libxl(u)
so that libvirt can benefit from it.

The bottom line is that client can opt out of this set of APIs if they
want to.

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

I don't think the location matters that much. You might 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.
> 
> I'm not sure what this paragraph means.

What Ian wanted was that we need clear correlation of xl configuration
syntax with libxl API fields. For example, for a FOO device

   xl.cfg: FOO = [ "bar=baz" ]

   libxl_types.idl:

    libxl_device_FOO = Struct("device_foo", [
      ("bar", some_type)
      ...
      ])

However, I don't think that can be easily achieved in the case of pvscsi
because we need to support both legacy Xen-classic kernel and new PVOPS
kernel. Olaf, please correct me if I'm wrong.


[...]
> +
> +static bool vscsi_fill_ctrl(libxl__gc *gc,
> +                            xs_transaction_t t,
> +                            const char *fe_path,
> +                            const char *dir,
> +                            libxl_device_vscsictrl *ctrl)
> +{
> +    libxl_device_vscsidev dev;
> +    char *tmp, *be_path, *devs_path;
> +    char **dev_dirs;
> +    unsigned int ndev_dirs, dev_dir;
> +    bool ok;
> +
> +    ctrl->devid = atoi(dir);
> +
> +    be_path = libxl__xs_read(gc, t, GCSPRINTF("%s/%s/backend", fe_path, 
> dir));
> +    if (!be_path)
> +        goto out;
> +
> +    tmp = libxl__xs_read(gc, t, GCSPRINTF("%s/%s/backend-id", fe_path, dir));
> +    if (!tmp)
> +        goto out;
> +    ctrl->backend_domid = atoi(tmp);

Please sanitise input coming from frontend.  You need to check if the
backend domid and frontend domid make sense.

Wei.

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