Re: [Xen-devel] [PATCH 3/3] docs: add more info about target= in disk config

2016-02-19 Thread Jim Fehlig
On 02/19/2016 10:23 AM, Ian Jackson wrote:
> Jim Fehlig writes ("[PATCH 3/3] docs: add more info about target= in disk 
> config"):
>> target= in disk config can be used to convey arbitrary
>> configuration information to backends. Add a bit more info
>> to xl-disk-configuration.txt to clarify this, including some
>> simple nbd and rbd qdisk configurations.
>> ---
>>  docs/misc/xl-disk-configuration.txt | 10 +-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/misc/xl-disk-configuration.txt 
>> b/docs/misc/xl-disk-configuration.txt
>> index 29f6ddb..0918fb8 100644
>> --- a/docs/misc/xl-disk-configuration.txt
>> +++ b/docs/misc/xl-disk-configuration.txt
>> @@ -75,7 +75,15 @@ Special syntax:
>> the target was already specified as a positional parameter.  This
>> is the only way to specify a target string containing metacharacters
>> such as commas and (in some cases) colons, which would otherwise be
>> -   misinterpreted.
>> +   misinterpreted. Meta-information in a target string can be used to
>> +   specify configuration information for a qdisk block backend. For
>> +   example the nbd and rbd qdisk block backends can be configured with
>> +
>> + target=nbd:192.168.0.1:
>> + target=rbd:pool/image:mon_host=192.186.0.1\\:6789
>> +
>> +   Note the use of double backslash ('\\') for metacharacters that need
>> +   escaped.
> I'm not entirely comfortable with documenting this as supported.
> The difficulties I see are:
>
>
> In the usual configuration, libxl decides for itself what (libxl)
> backend to use.  Different versions of libxl might make different
> choices, so a configuration that works with one version of libxl might
> not work with another.  That's fine for an undocumented feature but
> not so good if it's actually advertised.  At the very least the docs
> need to say that to rely on this you must specify backend=qdisk.

The text I added in this patch states that meta-information can be used with
qdisk. I didn't go as far as saying _only_ qdisk, since other backends might
interpret such meta-information too. But I can add that if we decide to go with
this doc patch.

>
>
> And this is a layering violation, or rather a violation of the
> expected semantics of the target string.
>
> I think it would be much better to support nbd and rbd explicitly in
> libxl.  Maybe we should have a "protocol=" parameter, so you could
> write something like this:
>disk=["vdev=xvda, protocol=nbd, target=192.168.0.1:"]
>
> This would allow libxl to make better choices about backends, even if
> right now all it does is force the use of the qemu backend and pass
> the target string to qemu.

I agree with your suggestion, which is why I took that approach in the original
RFC post

http://lists.xenproject.org/archives/html/xen-devel/2016-01/msg03184.html

Did you see the doc and IDL RFC patch attached to that post? IMO, we need more
than just protocol. An rbd device configuration can include a pool/volume name,
multiple servers, auth type, auth username, and auth passwd/data. E.g.

  disk = [ 'vdev=xvda, backendtype=qdisk, backendprotocol=rbd,
server=192.168.0.1:, server=192.168.0.2:, auth=joe:joes-secret,
target=some-pool/some-image' ]

>
>
> Finally, if this is actually true, it is a bug.  The specification
> (docs/misc/xl-disk-configuration.txt) says:
>
>   Description:   Block device or image file path.  When this is
>  used as a path, /dev will be prepended
>  if the path doesn't start with a '/'.

That is not true. I've successfully used the following disk config

  disk = [ "vdev=xvdb, backendtype=qdisk,
   
target=rbd:libvirtpool/image:auth_supported=none:mon_host=192.168.0.1\\:6789\\;192.168.0.2\\:6789\\;192.168.0.3\\:6789"
]

disk = [ "vdev=xvdb, backendtype=qdisk, target=nbd:192.168.0.1:" ]

I wouldn't call those targets "image file paths", but they don't start with a
'/' and none is prepended.

> See also what is said under `script='.

Do you mean how the 

Re: [Xen-devel] [PATCH 3/3] docs: add more info about target= in disk config

2016-02-19 Thread Ian Jackson
Jim Fehlig writes ("[PATCH 3/3] docs: add more info about target= in disk 
config"):
> target= in disk config can be used to convey arbitrary
> configuration information to backends. Add a bit more info
> to xl-disk-configuration.txt to clarify this, including some
> simple nbd and rbd qdisk configurations.
> ---
>  docs/misc/xl-disk-configuration.txt | 10 +-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/misc/xl-disk-configuration.txt 
> b/docs/misc/xl-disk-configuration.txt
> index 29f6ddb..0918fb8 100644
> --- a/docs/misc/xl-disk-configuration.txt
> +++ b/docs/misc/xl-disk-configuration.txt
> @@ -75,7 +75,15 @@ Special syntax:
> the target was already specified as a positional parameter.  This
> is the only way to specify a target string containing metacharacters
> such as commas and (in some cases) colons, which would otherwise be
> -   misinterpreted.
> +   misinterpreted. Meta-information in a target string can be used to
> +   specify configuration information for a qdisk block backend. For
> +   example the nbd and rbd qdisk block backends can be configured with
> +
> + target=nbd:192.168.0.1:
> + target=rbd:pool/image:mon_host=192.186.0.1\\:6789
> +
> +   Note the use of double backslash ('\\') for metacharacters that need
> +   escaped.

I'm not entirely comfortable with documenting this as supported.
The difficulties I see are:


In the usual configuration, libxl decides for itself what (libxl)
backend to use.  Different versions of libxl might make different
choices, so a configuration that works with one version of libxl might
not work with another.  That's fine for an undocumented feature but
not so good if it's actually advertised.  At the very least the docs
need to say that to rely on this you must specify backend=qdisk.


And this is a layering violation, or rather a violation of the
expected semantics of the target string.

I think it would be much better to support nbd and rbd explicitly in
libxl.  Maybe we should have a "protocol=" parameter, so you could
write something like this:
   disk=["vdev=xvda, protocol=nbd, target=192.168.0.1:"]

This would allow libxl to make better choices about backends, even if
right now all it does is force the use of the qemu backend and pass
the target string to qemu.


Finally, if this is actually true, it is a bug.  The specification
(docs/misc/xl-disk-configuration.txt) says:

  Description:   Block device or image file path.  When this is
 used as a path, /dev will be prepended
 if the path doesn't start with a '/'.

See also what is said under `script='.


Ian.

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


Re: [Xen-devel] [PATCH 3/3] docs: add more info about target= in disk config

2016-02-18 Thread Ian Campbell
On Wed, 2016-02-17 at 10:24 -0700, Jim Fehlig wrote:
> 
> > 
> > The general concept of escaping metacharaters is not mentioned in this doc
> > at all, i.e. there is no mention of which characters need such escaping nor
> > of the various "special" codes (\t and \n etc), nor of the octal and hex
> > escape codes. Maybe that's a topic for another patch though.
> 
> I could do that in a follow-up, but I have no clue what those mean or how they
> are used.

Fair enough. I've some spare cycles so I'll try and reverse engineer it and
write it down.

Ian.

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


Re: [Xen-devel] [PATCH 3/3] docs: add more info about target= in disk config

2016-02-17 Thread Jim Fehlig
On 02/17/2016 03:10 AM, Ian Campbell wrote:
> On Tue, 2016-02-16 at 20:54 -0700, Jim Fehlig wrote:
>> target= in disk config can be used to convey arbitrary
>> configuration information to backends. Add a bit more info
>> to xl-disk-configuration.txt to clarify this, including some
>> simple nbd and rbd qdisk configurations.
> Missing S-o-b.
>
>> ---
>>  docs/misc/xl-disk-configuration.txt | 10 +-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/misc/xl-disk-configuration.txt b/docs/misc/xl-disk-
>> configuration.txt
>> index 29f6ddb..0918fb8 100644
>> --- a/docs/misc/xl-disk-configuration.txt
>> +++ b/docs/misc/xl-disk-configuration.txt
>> @@ -75,7 +75,15 @@ Special syntax:
>> the target was already specified as a positional parameter.  This
>> is the only way to specify a target string containing metacharacters
>> such as commas and (in some cases) colons, which would otherwise be
>> -   misinterpreted.
>> +   misinterpreted. Meta-information in a target string can be used to
>> +   specify configuration information for a qdisk block backend. For
>> +   example the nbd and rbd qdisk block backends can be configured with
>> +
>> + target=nbd:192.168.0.1:
>> + target=rbd:pool/image:mon_host=192.186.0.1\\:6789
>> +
>> +   Note the use of double backslash ('\\') for metacharacters that need
>> +   escaped.
> "need to be escaped".
>
> However I wouldn't describe "\\" that way, I think I would say "note that \
> is used to escape metacharacters and therefore to get a literal backslash
> "\\" is required".

Agreed. I changed the text and sent a V2 of the series.

>
> The general concept of escaping metacharaters is not mentioned in this doc
> at all, i.e. there is no mention of which characters need such escaping nor
> of the various "special" codes (\t and \n etc), nor of the octal and hex
> escape codes. Maybe that's a topic for another patch though.

I could do that in a follow-up, but I have no clue what those mean or how they
are used.

Regards,
Jim


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


Re: [Xen-devel] [PATCH 3/3] docs: add more info about target= in disk config

2016-02-17 Thread Ian Campbell
On Tue, 2016-02-16 at 20:54 -0700, Jim Fehlig wrote:
> target= in disk config can be used to convey arbitrary
> configuration information to backends. Add a bit more info
> to xl-disk-configuration.txt to clarify this, including some
> simple nbd and rbd qdisk configurations.

Missing S-o-b.

> ---
>  docs/misc/xl-disk-configuration.txt | 10 +-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/misc/xl-disk-configuration.txt b/docs/misc/xl-disk-
> configuration.txt
> index 29f6ddb..0918fb8 100644
> --- a/docs/misc/xl-disk-configuration.txt
> +++ b/docs/misc/xl-disk-configuration.txt
> @@ -75,7 +75,15 @@ Special syntax:
> the target was already specified as a positional parameter.  This
> is the only way to specify a target string containing metacharacters
> such as commas and (in some cases) colons, which would otherwise be
> -   misinterpreted.
> +   misinterpreted. Meta-information in a target string can be used to
> +   specify configuration information for a qdisk block backend. For
> +   example the nbd and rbd qdisk block backends can be configured with
> +
> + target=nbd:192.168.0.1:
> + target=rbd:pool/image:mon_host=192.186.0.1\\:6789
> +
> +   Note the use of double backslash ('\\') for metacharacters that need
> +   escaped.

"need to be escaped".

However I wouldn't describe "\\" that way, I think I would say "note that \
is used to escape metacharacters and therefore to get a literal backslash
"\\" is required".

The general concept of escaping metacharaters is not mentioned in this doc
at all, i.e. there is no mention of which characters need such escaping nor
of the various "special" codes (\t and \n etc), nor of the octal and hex
escape codes. Maybe that's a topic for another patch though.

Ian.


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


[Xen-devel] [PATCH 3/3] docs: add more info about target= in disk config

2016-02-16 Thread Jim Fehlig
target= in disk config can be used to convey arbitrary
configuration information to backends. Add a bit more info
to xl-disk-configuration.txt to clarify this, including some
simple nbd and rbd qdisk configurations.
---
 docs/misc/xl-disk-configuration.txt | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/docs/misc/xl-disk-configuration.txt 
b/docs/misc/xl-disk-configuration.txt
index 29f6ddb..0918fb8 100644
--- a/docs/misc/xl-disk-configuration.txt
+++ b/docs/misc/xl-disk-configuration.txt
@@ -75,7 +75,15 @@ Special syntax:
the target was already specified as a positional parameter.  This
is the only way to specify a target string containing metacharacters
such as commas and (in some cases) colons, which would otherwise be
-   misinterpreted.
+   misinterpreted. Meta-information in a target string can be used to
+   specify configuration information for a qdisk block backend. For
+   example the nbd and rbd qdisk block backends can be configured with
+
+ target=nbd:192.168.0.1:
+ target=rbd:pool/image:mon_host=192.186.0.1\\:6789
+
+   Note the use of double backslash ('\\') for metacharacters that need
+   escaped.
 
Future parameter and flag names will start with an ascii letter and
contain only ascii alphanumerics, hyphens and underscores, and will
-- 
1.8.0.1


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