Xen devel - Mar 2013 - [PATCH] Fix cd-insert problem not detecting type.

String format used to parse file used target which does not handle
types. Append directly argument passes to command line so parser
handle type correctly.

The issue was caused by setting pdev_path with type:file which was
used to stat the file.

Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
---
 tools/libxl/xl_cmdimpl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index a98705e..5d4fb39 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2519,7 +2519,7 @@ static void cd_insert(uint32_t domid, const char
*virtdev, char *phys)
     XLU_Config *config = 0;


-    if (asprintf(&buf,
"vdev=%s,access=r,devtype=cdrom,target=%s",
+    if (asprintf(&buf, "vdev=%s,access=r,devtype=cdrom,%s",
                  virtdev, phys ? phys : "") < 0) {
         fprintf(stderr, "out of memory\n");
         return;
-- 
1.7.10.4

Frediano Ziglio writes ("[PATCH] Fix cd-insert problem not detecting
type."):
> String format used to parse file used target which does not handle
> types. Append directly argument passes to command line so parser
> handle type correctly.
Thanks for pointing out this problem.

The problem here seems to be that "xl cd-insert" takes a
<target>.
This is documented.  Your change would break some existing setups
(where the <target> passed contained certain metacharacters).

But I think the current definition of "xl cd-insert" is wrongheaded.
It should surely take a whole block device specification, with a
special option passed to the parser to force "cdrom", and another
special option passed to the parser to stop it failing if vdev isn''t
specified.

The caller should check that the vdev is either unspecified or
specifies the same device.

In any case, I think the approach in the existing code of trying to
unparse the user-supplied target into a string for the benefit of
parse_disk_config is wrong.

What do other people think about this ?

Ian.

On Mon, 2013-03-11 at 16:17 +0000, Ian Jackson wrote:
> Frediano Ziglio writes ("[PATCH] Fix cd-insert problem not detecting
type."):
> > String format used to parse file used target which does not handle
> > types. Append directly argument passes to command line so parser
> > handle type correctly.
> 
> Thanks for pointing out this problem.
> 
> The problem here seems to be that "xl cd-insert" takes a
<target>.
> This is documented.  Your change would break some existing setups
> (where the <target> passed contained certain metacharacters).
> 
> But I think the current definition of "xl cd-insert" is
wrongheaded.
> It should surely take a whole block device specification, with a
> special option passed to the parser to force "cdrom", and another
> special option passed to the parser to stop it failing if vdev
isn''t
> specified.
> 
> The caller should check that the vdev is either unspecified or
> specifies the same device.
> 
> In any case, I think the approach in the existing code of trying to
> unparse the user-supplied target into a string for the benefit of
> parse_disk_config is wrong.
> 
> What do other people think about this ?
> 
> Ian.
Just an addition,
  Fabio Fantoni (first reporter of this issue) say that on his side
patch is not working (it worked for me and I was able to change the cd
with an iso image). For some reason his setup try to use a file as a
physical device.

I think that changing XLU code (libxlu_disk_l.l) could just make some
other code break or move the parser back. I think we should keep the
command line syntax compatibility while updating internal code so
perhaps the better option is to detect is user tried to give a specific
type and add the correct "format" option to pass to parse_disk_config.
If you agree with this I can write a patch. Is not clear however which
"types" are supported for xl cd-insert command. It seems a mix of
backend (like "phy") and formats (like "raw"). Is this
expected?

Frediano

Frediano Ziglio writes ("Re: [PATCH] Fix cd-insert problem not detecting
type."):
> I think that changing XLU code (libxlu_disk_l.l) could just make some
> other code break or move the parser back.
I don''t think we need to substantially change the existing parser.  I
think we need to make sure we use it in the normal way.
>   I think we should keep the
> command line syntax compatibility
Well, currently the command line syntax doesn''t support
"raw:<blah>"
etc. which is what you are trying to introduce.  "raw:<blah>"
would
mean a file whose name starts "raw..." etc.  So it is not possible to
make your use case work without breaking some obscure cases.

My proposal is that we should bite this bullet and regularise the
parsing, so that xl cd-insert takes the same kind of argument as xl
block-attach.
>   while updating internal code so perhaps the better option is to
> detect is user tried to give a specific type and add the correct
> "format" option to pass to parse_disk_config.  If you agree with
> this I can write a patch. Is not clear however which "types" are
> supported for xl cd-insert command. It seems a mix of backend (like
> "phy") and formats (like "raw"). Is this expected?
These "types" are a xend thing and are indeed a mixture of backend,
format, and other information.  In xl they are supported (where they
are) for backward compatibility, but they are not recommended.

Ian.

On Mon, 2013-03-11 at 17:38 +0000, Ian Jackson wrote:
> Frediano Ziglio writes ("Re: [PATCH] Fix cd-insert problem not
detecting type."):
> > I think that changing XLU code (libxlu_disk_l.l) could just make some
> > other code break or move the parser back.
> 
> I don''t think we need to substantially change the existing parser.
I
> think we need to make sure we use it in the normal way.
> 
> >   I think we should keep the
> > command line syntax compatibility
> 
> Well, currently the command line syntax doesn''t support
"raw:<blah>"
> etc. which is what you are trying to introduce. 
"raw:<blah>" would
> mean a file whose name starts "raw..." etc.  So it is not
possible to
> make your use case work without breaking some obscure cases.
> 
In 4.1 you could use prefixes that using a regex are

(phy|file|tap2?:(aio:)?(vhd|qcow|qcow2|raw)):

you could use similar syntax in Xml file. If you run just "xl
cd-insert"
output is


''xl cd-insert'' requires at least 3 arguments.

Usage: xl [-vfN] cd-insert <Domain> <VirtualDevice>
<type:path>

Insert a cdrom into a guest''s cd drive.


so anyway user still expect a similar syntax. block-attach support a
different syntax where parameters are split using different command line
arguments. Now it seems that code was changed trying to embed old syntax
in new parameters way but target parameter was used which does not
support old syntax.

It seems that libxlu_disk_l.l try even to emulate the old syntax but it
does not set correctly "old" format and backend, for instance for raw
should set backend to tap and format to raw while it just set format.
> My proposal is that we should bite this bullet and regularise the
> parsing, so that xl cd-insert takes the same kind of argument as xl
> block-attach.
> 
> >   while updating internal code so perhaps the better option is to
> > detect is user tried to give a specific type and add the correct
> > "format" option to pass to parse_disk_config.  If you agree
with
> > this I can write a patch. Is not clear however which "types"
are
> > supported for xl cd-insert command. It seems a mix of backend (like
> > "phy") and formats (like "raw"). Is this expected?
> 
> These "types" are a xend thing and are indeed a mixture of
backend,
> format, and other information.  In xl they are supported (where they
> are) for backward compatibility, but they are not recommended.
> 
> Ian.
By the way, do we want to break cd-insert command line syntax or just
add a new format?
Or is it better to add a new "block-change" command?
Currently using 4.2 you can''t specify as many format as 4.1.

Frediano

On Tue, 12 Mar 2013, Frediano Ziglio wrote:
> In 4.1 you could use prefixes that using a regex are
> 
> (phy|file|tap2?:(aio:)?(vhd|qcow|qcow2|raw)):
> 
> you could use similar syntax in Xml file. If you run just "xl
cd-insert"
> output is
XML file? Are you sure that you are talking about xl and xm or something
else?

> ''xl cd-insert'' requires at least 3 arguments.
> 
> Usage: xl [-vfN] cd-insert <Domain> <VirtualDevice>
<type:path>
> 
> Insert a cdrom into a guest''s cd drive.
> 
> 
> so anyway user still expect a similar syntax. block-attach support a
> different syntax where parameters are split using different command line
> arguments. Now it seems that code was changed trying to embed old syntax
> in new parameters way but target parameter was used which does not
> support old syntax.
> 
> It seems that libxlu_disk_l.l try even to emulate the old syntax but it
> does not set correctly "old" format and backend, for instance for
raw
> should set backend to tap and format to raw while it just set format.
That is correct because libxl is able to figure out which one is the
best backend for "raw", that nowadays usually ends up being qemu.

> > My proposal is that we should bite this bullet and regularise the
> > parsing, so that xl cd-insert takes the same kind of argument as xl
> > block-attach.
I don''t know if supporting the full disk-spec-component syntax is the
best thing to do here: for instance you are not supposed to pass a
devtype different from cdrom.
At the very least we need to clarify that devtype is not respected
when passed to cd-insert.

> > >   while updating internal code so perhaps the better option is to
> > > detect is user tried to give a specific type and add the correct
> > > "format" option to pass to parse_disk_config.  If you
agree with
> > > this I can write a patch. Is not clear however which
"types" are
> > > supported for xl cd-insert command. It seems a mix of backend
(like
> > > "phy") and formats (like "raw"). Is this
expected?
> > 
> > These "types" are a xend thing and are indeed a mixture of
backend,
> > format, and other information.  In xl they are supported (where they
> > are) for backward compatibility, but they are not recommended.
> > 
> 
> By the way, do we want to break cd-insert command line syntax or just
> add a new format?
> Or is it better to add a new "block-change" command?
> Currently using 4.2 you can''t specify as many format as 4.1.
I would hate to have to introduce yet another command to work around a
broken one.
I would rather break compatibility with the existing cd-insert.

On Fri, 2013-03-15 at 14:51 +0000, Stefano Stabellini
wrote:
> On Tue, 12 Mar 2013, Frediano Ziglio wrote:
> > In 4.1 you could use prefixes that using a regex are
> > 
> > (phy|file|tap2?:(aio:)?(vhd|qcow|qcow2|raw)):
> > 
> > you could use similar syntax in Xml file. If you run just "xl
cd-insert"
> > output is
> 
> XML file? Are you sure that you are talking about xl and xm or something
> else?
> 
Was xl configuration files.
> 
> > ''xl cd-insert'' requires at least 3 arguments.
> > 
> > Usage: xl [-vfN] cd-insert <Domain> <VirtualDevice>
<type:path>
> > 
> > Insert a cdrom into a guest''s cd drive.
> > 
> > 
> > so anyway user still expect a similar syntax. block-attach support a
> > different syntax where parameters are split using different command
line
> > arguments. Now it seems that code was changed trying to embed old
syntax
> > in new parameters way but target parameter was used which does not
> > support old syntax.
> > 
> > It seems that libxlu_disk_l.l try even to emulate the old syntax but
it
> > does not set correctly "old" format and backend, for
instance for raw
> > should set backend to tap and format to raw while it just set format.
> 
> That is correct because libxl is able to figure out which one is the
> best backend for "raw", that nowadays usually ends up being qemu.
> 
> 
> > > My proposal is that we should bite this bullet and regularise the
> > > parsing, so that xl cd-insert takes the same kind of argument as
xl
> > > block-attach.
> 
> I don''t know if supporting the full disk-spec-component syntax is
the
> best thing to do here: for instance you are not supposed to pass a
> devtype different from cdrom.
> At the very least we need to clarify that devtype is not respected
> when passed to cd-insert.
> 
> 
> > > >   while updating internal code so perhaps the better option
is to
> > > > detect is user tried to give a specific type and add the
correct
> > > > "format" option to pass to parse_disk_config.  If
you agree with
> > > > this I can write a patch. Is not clear however which
"types" are
> > > > supported for xl cd-insert command. It seems a mix of
backend (like
> > > > "phy") and formats (like "raw"). Is this
expected?
> > > 
> > > These "types" are a xend thing and are indeed a mixture
of backend,
> > > format, and other information.  In xl they are supported (where
they
> > > are) for backward compatibility, but they are not recommended.
> > > 
> > 
> > By the way, do we want to break cd-insert command line syntax or just
> > add a new format?
> > Or is it better to add a new "block-change" command?
> > Currently using 4.2 you can''t specify as many format as 4.1.
> 
> I would hate to have to introduce yet another command to work around a
> broken one.
> I would rather break compatibility with the existing cd-insert.
I would be all right with you if we removed supported for this syntax
but as xl configurations files are still supported with this syntax I
prefer to not change cd-insert one.

Frediano

Stefano Stabellini writes ("Re: [PATCH] Fix cd-insert problem not detecting
type."):
> I don''t know if supporting the full disk-spec-component syntax is
the
> best thing to do here:
I think regularity would be very valuable.
>  for instance you are not supposed to pass a devtype different from
> cdrom.  At the very least we need to clarify that devtype is not
> respected when passed to cd-insert.
The devtype should be checked, and devtypes other than cdrom should
produce an error.  In xl cd-insert it should default to cdrom, so this
isn''t an error you''d normallys ee.
> I would hate to have to introduce yet another command to work around a
> broken one.
Quite.
> I would rather break compatibility with the existing cd-insert.
Yes.

Ian.

On Fri, 15 Mar 2013, Frediano Ziglio wrote:
> On Fri, 2013-03-15 at 14:51 +0000, Stefano Stabellini wrote:
> > On Tue, 12 Mar 2013, Frediano Ziglio wrote:
> > > In 4.1 you could use prefixes that using a regex are
> > > 
> > > (phy|file|tap2?:(aio:)?(vhd|qcow|qcow2|raw)):
> > > 
> > > you could use similar syntax in Xml file. If you run just
"xl cd-insert"
> > > output is
> > 
> > XML file? Are you sure that you are talking about xl and xm or
something
> > else?
> > 
> 
> Was xl configuration files.
I didn''t know that something like 

(phy|file|tap2?:(aio:)?(vhd|qcow|qcow2|raw))

is supported in the VM  config file.
Maybe with xend, but certainly it can''t be the case with XL.


> > > By the way, do we want to break cd-insert command line syntax or
just
> > > add a new format?
> > > Or is it better to add a new "block-change" command?
> > > Currently using 4.2 you can''t specify as many format as
4.1.
> > 
> > I would hate to have to introduce yet another command to work around a
> > broken one.
> > I would rather break compatibility with the existing cd-insert.
> 
> I would be all right with you if we removed supported for this syntax
> but as xl configurations files are still supported with this syntax I
> prefer to not change cd-insert one.
Sorry but I can''t parse what you wrote.
Also, as I wrote above, I don''t think that

(phy|file|tap2?:(aio:)?(vhd|qcow|qcow2|raw))

is supported (or should be supported) by xl

On Tue, 2013-03-19 at 13:48 +0000, Stefano Stabellini
wrote:
> On Fri, 15 Mar 2013, Frediano Ziglio wrote:
> > On Fri, 2013-03-15 at 14:51 +0000, Stefano Stabellini wrote:
> > > On Tue, 12 Mar 2013, Frediano Ziglio wrote:
> > > > In 4.1 you could use prefixes that using a regex are
> > > > 
> > > > (phy|file|tap2?:(aio:)?(vhd|qcow|qcow2|raw)):
> > > > 
> > > > you could use similar syntax in Xml file. If you run just
"xl cd-insert"
> > > > output is
> > > 
> > > XML file? Are you sure that you are talking about xl and xm or
something
> > > else?
> > > 
> > 
> > Was xl configuration files.
> 
> I didn''t know that something like 
> 
> (phy|file|tap2?:(aio:)?(vhd|qcow|qcow2|raw))
> 
> is supported in the VM  config file.
> Maybe with xend, but certainly it can''t be the case with XL.
> 
> 
Look at parse_disk_config in tools/libxl/xl_cmdimpl.c in 4.1 release.
> 
> > > > By the way, do we want to break cd-insert command line
syntax or just
> > > > add a new format?
> > > > Or is it better to add a new "block-change"
command?
> > > > Currently using 4.2 you can''t specify as many
format as 4.1.
> > > 
> > > I would hate to have to introduce yet another command to work
around a
> > > broken one.
> > > I would rather break compatibility with the existing cd-insert.
> > 
> > I would be all right with you if we removed supported for this syntax
> > but as xl configurations files are still supported with this syntax I
> > prefer to not change cd-insert one.
> 
> Sorry but I can''t parse what you wrote.
> Also, as I wrote above, I don''t think that
> 
> (phy|file|tap2?:(aio:)?(vhd|qcow|qcow2|raw))
> 
> is supported (or should be supported) by xl
Was supported in 4.1 and still xl command state that syntax is
type:path. Configuration file still accept syntax like
''raw:/mnt/win7-x86.iso,hdd:cdrom,r'' for disks even on last xen
unstable.

Frediano

On Tue, 2013-03-19 at 13:48 +0000, Stefano Stabellini
wrote:
> On Fri, 15 Mar 2013, Frediano Ziglio wrote:
> > On Fri, 2013-03-15 at 14:51 +0000, Stefano Stabellini wrote:
> > > On Tue, 12 Mar 2013, Frediano Ziglio wrote:
> > > > In 4.1 you could use prefixes that using a regex are
> > > > 
> > > > (phy|file|tap2?:(aio:)?(vhd|qcow|qcow2|raw)):
> > > > 
> > > > you could use similar syntax in Xml file. If you run just
"xl cd-insert"
> > > > output is
> > > 
> > > XML file? Are you sure that you are talking about xl and xm or
something
> > > else?
> > > 
> > 
> > Was xl configuration files.
> 
> I didn''t know that something like 
> 
> (phy|file|tap2?:(aio:)?(vhd|qcow|qcow2|raw))
> 
> is supported in the VM  config file.
> Maybe with xend, but certainly it can''t be the case with XL.
For compatibility the phy/tap2/file ones are "supported" to the extent
that xl will accept but ignore them while the vhd/qcow/etc ones are
accepted and have the expected effect.

docs/misc/xl-disk-configuration.txt has the details. It does mention the
eventual potential removal of the deprecated prefixes, but AFAIK we
haven''t done that.

Ian.