* [PATCH 0/3] libxl and docs: small improvements in qdisk support
@ 2016-02-17 3:54 Jim Fehlig
2016-02-17 3:54 ` [PATCH 1/3] libxlu_cfg: reject unknown characters following '\' Jim Fehlig
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Jim Fehlig @ 2016-02-17 3:54 UTC (permalink / raw)
To: xen-devel; +Cc: wei.liu2, ian.jackson, ian.campbell
This series contains a few improvments related to libxl's
support for the various qdisk types. Patch1 is a small fix
for libxlu_cfg to error when encountering unknown backslash-
escaped characters instead of silently dropping them. Patch2
is actually unrelated and fixes a typo noticed while reviewing
xl-disk-configuration.txt, which is improved a bit in patch3
wrt target= syntax.
Jim Fehlig (3):
libxlu_cfg: reject unknown characters following '\'
docs: fix typo in xl-disk-configuration.txt
docs: add more info about target= in disk config
docs/misc/xl-disk-configuration.txt | 12 ++++++++++--
tools/libxl/libxlu_cfg.c | 5 +++++
2 files changed, 15 insertions(+), 2 deletions(-)
--
1.8.0.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/3] libxlu_cfg: reject unknown characters following '\'
2016-02-17 3:54 [PATCH 0/3] libxl and docs: small improvements in qdisk support Jim Fehlig
@ 2016-02-17 3:54 ` Jim Fehlig
2016-02-17 10:05 ` Ian Campbell
2016-02-17 3:54 ` [PATCH 2/3] docs: fix typo in xl-disk-configuration.txt Jim Fehlig
2016-02-17 3:54 ` [PATCH 3/3] docs: add more info about target= in disk config Jim Fehlig
2 siblings, 1 reply; 14+ messages in thread
From: Jim Fehlig @ 2016-02-17 3:54 UTC (permalink / raw)
To: xen-devel; +Cc: wei.liu2, ian.jackson, ian.campbell
When dequoting config strings in xlu__cfgl_dequote(), unknown
characters following a '\', and the '\' itself, are discarded.
E.g. a disk configuration string containing
rbd:pool/image:mon_host=192.168.0.100\:6789
would be dequoted as
rbd:pool/image:mon_host=192.168.0.1006789
Instead of discarding the '\' and unknown character, reject the
string and set error to EINVAL.
---
tools/libxl/libxlu_cfg.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/tools/libxl/libxlu_cfg.c b/tools/libxl/libxlu_cfg.c
index 1d70909..f8e0bc7 100644
--- a/tools/libxl/libxlu_cfg.c
+++ b/tools/libxl/libxlu_cfg.c
@@ -533,6 +533,11 @@ char *xlu__cfgl_dequote(CfgParseContext *ctx, const char *src) {
NUMERIC_CHAR(2,2,16,"hex");
} else if (nc>='0' && nc<='7') {
NUMERIC_CHAR(1,3,10,"octal");
+ } else {
+ xlu__cfgl_lexicalerror(ctx, "invalid character after backlash "
+ "in quoted string");
+ ctx->err= EINVAL;
+ goto x;
}
assert(p <= src+len-1);
} else {
--
1.8.0.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/3] docs: fix typo in xl-disk-configuration.txt
2016-02-17 3:54 [PATCH 0/3] libxl and docs: small improvements in qdisk support Jim Fehlig
2016-02-17 3:54 ` [PATCH 1/3] libxlu_cfg: reject unknown characters following '\' Jim Fehlig
@ 2016-02-17 3:54 ` Jim Fehlig
2016-02-17 10:05 ` Ian Campbell
2016-02-17 3:54 ` [PATCH 3/3] docs: add more info about target= in disk config Jim Fehlig
2 siblings, 1 reply; 14+ messages in thread
From: Jim Fehlig @ 2016-02-17 3:54 UTC (permalink / raw)
To: xen-devel; +Cc: wei.liu2, ian.jackson, ian.campbell
---
docs/misc/xl-disk-configuration.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/docs/misc/xl-disk-configuration.txt b/docs/misc/xl-disk-configuration.txt
index 6a2118d..29f6ddb 100644
--- a/docs/misc/xl-disk-configuration.txt
+++ b/docs/misc/xl-disk-configuration.txt
@@ -160,7 +160,7 @@ Mandatory: No
Default value: Automatically determine which backend to use.
This does not affect the guest's view of the device. It controls
-which software implementation of the Xen backend driver us used.
+which software implementation of the Xen backend driver is used.
Not all backend drivers support all combinations of other options.
For example, "phy" does not support formats other than "raw".
--
1.8.0.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/3] docs: add more info about target= in disk config
2016-02-17 3:54 [PATCH 0/3] libxl and docs: small improvements in qdisk support Jim Fehlig
2016-02-17 3:54 ` [PATCH 1/3] libxlu_cfg: reject unknown characters following '\' Jim Fehlig
2016-02-17 3:54 ` [PATCH 2/3] docs: fix typo in xl-disk-configuration.txt Jim Fehlig
@ 2016-02-17 3:54 ` Jim Fehlig
2016-02-17 10:10 ` Ian Campbell
2016-02-19 17:23 ` Ian Jackson
2 siblings, 2 replies; 14+ messages in thread
From: Jim Fehlig @ 2016-02-17 3:54 UTC (permalink / raw)
To: xen-devel; +Cc: wei.liu2, ian.jackson, ian.campbell
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:5555
+ 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] libxlu_cfg: reject unknown characters following '\'
2016-02-17 3:54 ` [PATCH 1/3] libxlu_cfg: reject unknown characters following '\' Jim Fehlig
@ 2016-02-17 10:05 ` Ian Campbell
2016-02-17 10:11 ` Ian Campbell
0 siblings, 1 reply; 14+ messages in thread
From: Ian Campbell @ 2016-02-17 10:05 UTC (permalink / raw)
To: Jim Fehlig, xen-devel; +Cc: wei.liu2, ian.jackson
On Tue, 2016-02-16 at 20:54 -0700, Jim Fehlig wrote:
> When dequoting config strings in xlu__cfgl_dequote(), unknown
> characters following a '\', and the '\' itself, are discarded.
> E.g. a disk configuration string containing
>
> rbd:pool/image:mon_host=192.168.0.100\:6789
>
> would be dequoted as
>
> rbd:pool/image:mon_host=192.168.0.1006789
>
> Instead of discarding the '\' and unknown character, reject the
> string and set error to EINVAL.
Missing your S-o-b.
Other than that:
> + xlu__cfgl_lexicalerror(ctx, "invalid character after backlash "
> + "in quoted string");
Please try where possible not to split string constants (so log messages
can more easily be grepped for). If that would result in more than 80
characters then it is acceptable to pull the string in more than you would
normally, e.g if this is (as I expect it will be) too long:
xlu__cfgl_lexicalerror(ctx,
"invalid character after backlash in quoted string");
then it's ok to do:
xlu__cfgl_lexicalerror(ctx,
"invalid
character after backlash in quoted string");
(I usually try and keep it to a multiple of 4 spaces indent, FWIW) or for really long strings even:
xlu__cfgl_lexicalerror(ctx,
"invalid character after backlash in quoted string blah blah blah");
is preferable to breaking the string in half.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] docs: fix typo in xl-disk-configuration.txt
2016-02-17 3:54 ` [PATCH 2/3] docs: fix typo in xl-disk-configuration.txt Jim Fehlig
@ 2016-02-17 10:05 ` Ian Campbell
0 siblings, 0 replies; 14+ messages in thread
From: Ian Campbell @ 2016-02-17 10:05 UTC (permalink / raw)
To: Jim Fehlig, xen-devel; +Cc: wei.liu2, ian.jackson
On Tue, 2016-02-16 at 20:54 -0700, Jim Fehlig wrote:
Missing S-o-b, otherwise:
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> docs/misc/xl-disk-configuration.txt | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/docs/misc/xl-disk-configuration.txt b/docs/misc/xl-disk-
> configuration.txt
> index 6a2118d..29f6ddb 100644
> --- a/docs/misc/xl-disk-configuration.txt
> +++ b/docs/misc/xl-disk-configuration.txt
> @@ -160,7 +160,7 @@ Mandatory: No
> Default value: Automatically determine which backend to use.
>
> This does not affect the guest's view of the device. It controls
> -which software implementation of the Xen backend driver us used.
> +which software implementation of the Xen backend driver is used.
>
> Not all backend drivers support all combinations of other options.
> For example, "phy" does not support formats other than "raw".
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] docs: add more info about target= in disk config
2016-02-17 3:54 ` [PATCH 3/3] docs: add more info about target= in disk config Jim Fehlig
@ 2016-02-17 10:10 ` Ian Campbell
2016-02-17 17:24 ` Jim Fehlig
2016-02-19 17:23 ` Ian Jackson
1 sibling, 1 reply; 14+ messages in thread
From: Ian Campbell @ 2016-02-17 10:10 UTC (permalink / raw)
To: Jim Fehlig, xen-devel; +Cc: wei.liu2, ian.jackson
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:5555
> + 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] libxlu_cfg: reject unknown characters following '\'
2016-02-17 10:05 ` Ian Campbell
@ 2016-02-17 10:11 ` Ian Campbell
2016-02-17 17:26 ` Jim Fehlig
0 siblings, 1 reply; 14+ messages in thread
From: Ian Campbell @ 2016-02-17 10:11 UTC (permalink / raw)
To: Jim Fehlig, xen-devel; +Cc: wei.liu2, ian.jackson
On Wed, 2016-02-17 at 10:05 +0000, Ian Campbell wrote:
> On Tue, 2016-02-16 at 20:54 -0700, Jim Fehlig wrote:
> > When dequoting config strings in xlu__cfgl_dequote(), unknown
> > characters following a '\', and the '\' itself, are discarded.
> > E.g. a disk configuration string containing
> >
> > rbd:pool/image:mon_host=192.168.0.100\:6789
> >
> > would be dequoted as
> >
> > rbd:pool/image:mon_host=192.168.0.1006789
> >
> > Instead of discarding the '\' and unknown character, reject the
> > string and set error to EINVAL.
>
> Missing your S-o-b.
>
> Other than that:
>
> > + xlu__cfgl_lexicalerror(ctx, "invalid character after
> > backlash "
> > + "in quoted string");
>
> Please try where possible not to split string constants (so log messages
> can more easily be grepped for).
I see now that this parsing code is pretty liberally ignoring this advice
already. So apart from the missing S-o-b this patch is
Acked-by: Ian Campbell < ian.campbell@citrix.com >
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] docs: add more info about target= in disk config
2016-02-17 10:10 ` Ian Campbell
@ 2016-02-17 17:24 ` Jim Fehlig
2016-02-18 10:22 ` Ian Campbell
0 siblings, 1 reply; 14+ messages in thread
From: Jim Fehlig @ 2016-02-17 17:24 UTC (permalink / raw)
To: Ian Campbell, xen-devel; +Cc: wei.liu2, ian.jackson
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:5555
>> + 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] libxlu_cfg: reject unknown characters following '\'
2016-02-17 10:11 ` Ian Campbell
@ 2016-02-17 17:26 ` Jim Fehlig
0 siblings, 0 replies; 14+ messages in thread
From: Jim Fehlig @ 2016-02-17 17:26 UTC (permalink / raw)
To: Ian Campbell, xen-devel; +Cc: wei.liu2, ian.jackson
On 02/17/2016 03:11 AM, Ian Campbell wrote:
> On Wed, 2016-02-17 at 10:05 +0000, Ian Campbell wrote:
>> On Tue, 2016-02-16 at 20:54 -0700, Jim Fehlig wrote:
>>> When dequoting config strings in xlu__cfgl_dequote(), unknown
>>> characters following a '\', and the '\' itself, are discarded.
>>> E.g. a disk configuration string containing
>>>
>>> rbd:pool/image:mon_host=192.168.0.100\:6789
>>>
>>> would be dequoted as
>>>
>>> rbd:pool/image:mon_host=192.168.0.1006789
>>>
>>> Instead of discarding the '\' and unknown character, reject the
>>> string and set error to EINVAL.
>> Missing your S-o-b.
>>
>> Other than that:
>>
>>> + xlu__cfgl_lexicalerror(ctx, "invalid character after
>>> backlash "
>>> + "in quoted string");
>> Please try where possible not to split string constants (so log messages
>> can more easily be grepped for).
> I see now that this parsing code is pretty liberally ignoring this advice
> already. So apart from the missing S-o-b this patch is
Your suggestion is a good one and is common throughout much of libxl, so I
pulled back the error string indentation in V2.
Regards,
Jim
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] docs: add more info about target= in disk config
2016-02-17 17:24 ` Jim Fehlig
@ 2016-02-18 10:22 ` Ian Campbell
0 siblings, 0 replies; 14+ messages in thread
From: Ian Campbell @ 2016-02-18 10:22 UTC (permalink / raw)
To: Jim Fehlig, xen-devel; +Cc: wei.liu2, ian.jackson
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] docs: add more info about target= in disk config
2016-02-17 3:54 ` [PATCH 3/3] docs: add more info about target= in disk config Jim Fehlig
2016-02-17 10:10 ` Ian Campbell
@ 2016-02-19 17:23 ` Ian Jackson
2016-02-19 19:06 ` Jim Fehlig
1 sibling, 1 reply; 14+ messages in thread
From: Ian Jackson @ 2016-02-19 17:23 UTC (permalink / raw)
To: Jim Fehlig; +Cc: wei.liu2, ian.campbell, xen-devel
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:5555
> + 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:5555"]
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] docs: add more info about target= in disk config
2016-02-19 17:23 ` Ian Jackson
@ 2016-02-19 19:06 ` Jim Fehlig
2016-02-23 9:52 ` Ian Campbell
0 siblings, 1 reply; 14+ messages in thread
From: Jim Fehlig @ 2016-02-19 19:06 UTC (permalink / raw)
To: Ian Jackson; +Cc: wei.liu2, ian.campbell, xen-devel
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:5555
>> + 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:5555"]
>
> 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:5555, server=192.168.0.2:5555, 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:5555" ]
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 <script> path is determined? Or the relationship between
<script> and <target>?
Regards,
Jim
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] docs: add more info about target= in disk config
2016-02-19 19:06 ` Jim Fehlig
@ 2016-02-23 9:52 ` Ian Campbell
0 siblings, 0 replies; 14+ messages in thread
From: Ian Campbell @ 2016-02-23 9:52 UTC (permalink / raw)
To: Jim Fehlig, Ian Jackson; +Cc: wei.liu2, xen-devel
On Fri, 2016-02-19 at 12:06 -0700, Jim Fehlig wrote:
> 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:5555
> > > + 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:5555"]
> >
> > 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:5555, server=192.168.0.2:5555, 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\\:6
> 789\\;192.168.0.2\\:6789\\;192.168.0.3\\:6789"
> ]
>
> disk = [ "vdev=xvdb, backendtype=qdisk, target=nbd:192.168.0.1:5555" ]
>
> I wouldn't call those targets "image file paths", but they don't start
> with a
> '/' and none is prepended.
It has also been long expected that one can pass an iSCSI target= here.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2016-02-23 9:52 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-02-17 3:54 [PATCH 0/3] libxl and docs: small improvements in qdisk support Jim Fehlig
2016-02-17 3:54 ` [PATCH 1/3] libxlu_cfg: reject unknown characters following '\' Jim Fehlig
2016-02-17 10:05 ` Ian Campbell
2016-02-17 10:11 ` Ian Campbell
2016-02-17 17:26 ` Jim Fehlig
2016-02-17 3:54 ` [PATCH 2/3] docs: fix typo in xl-disk-configuration.txt Jim Fehlig
2016-02-17 10:05 ` Ian Campbell
2016-02-17 3:54 ` [PATCH 3/3] docs: add more info about target= in disk config Jim Fehlig
2016-02-17 10:10 ` Ian Campbell
2016-02-17 17:24 ` Jim Fehlig
2016-02-18 10:22 ` Ian Campbell
2016-02-19 17:23 ` Ian Jackson
2016-02-19 19:06 ` Jim Fehlig
2016-02-23 9:52 ` Ian Campbell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).