# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1343211245 -3600
# Node ID 576aeedaca777236db8760ac5ca17b30f1c5354d
# Parent f0249f4b34e3ca3a770a09c688f9c1d1c523f5e7
xl: support empty CDROM devices
The important change here is to xlu_disk_parse to correctly set format == EMPTY
for CDROM devices which are empty. Test cases are added which check for
correctness here.
xend accepts '',hdc:cdrom,r''[0] as an empty CDROM drive however
this is not
consistent with the xl syntax in docs/misc/xl-disk-configuration.txt which
requires '',,hdc:cdrom,r'' (the additional positional paramter
is the format).
I''m not sure if/how this can be fixed. Note that xend does not accept
'',,hdc:cdrom,r''
There are several incidental cleanups included the the cdrom-{insert,eject}
commands:
- add a dry-run mode
- use the non-deprecated disk specification syntax
- check for and report errors from libxl_cdrom_insert
[0] http://wiki.xen.org/wiki/CD_Rom_Support_in_Xen
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
diff -r f0249f4b34e3 -r 576aeedaca77 docs/man/xl.pod.1
--- a/docs/man/xl.pod.1 Wed Jul 25 10:23:41 2012 +0100
+++ b/docs/man/xl.pod.1 Wed Jul 25 11:14:05 2012 +0100
@@ -1037,9 +1037,12 @@ in the domain.
List virtual block devices for a domain.
-=item B<cd-insert> I<domain-id> I<VirtualDevice>
I<be-dev>
+=item B<cd-insert> I<domain-id> I<VirtualDevice>
I<target>
-Insert a cdrom into a guest domain''s cd drive. Only works with HVM
domains.
+Insert a cdrom into a guest domain''s existing virtial cd drive. The
+virtual drive must already exist but can be current empty.
+
+Only works with HVM domains.
B<OPTIONS>
@@ -1047,20 +1050,29 @@ B<OPTIONS>
=item I<VirtualDevice>
-How the device should be presented to the guest domain; for example /dev/hdc.
+How the device should be presented to the guest domain; for example
"hdc".
-=item I<be-dev>
+=item I<target>
-the device in the backend domain (usually domain 0) to be exported; it
-can be a path to a file (file://path/to/file.iso). See B<disk> in
-L<xl.cfg(5)> for the details.
+the target path in the backend domain (usually domain 0) to be
+exported; Can be a block device or a file etc. See B<target> in
+F<docs/misc/xl-disk-configuration.txt>.
=back
=item B<cd-eject> I<domain-id> I<VirtualDevice>
-Eject a cdrom from a guest''s cd drive. Only works with HVM domains.
-I<VirtualDevice> is the cdrom device in the guest to eject.
+Eject a cdrom from a guest''s virtual cd drive. Only works with HVM
domains.
+
+B<OPTIONS>
+
+=over 4
+
+=item I<VirtualDevice>
+
+How the device should be presented to the guest domain; for example
"hdc".
+
+=back
=back
diff -r f0249f4b34e3 -r 576aeedaca77 tools/libxl/check-xl-disk-parse
--- a/tools/libxl/check-xl-disk-parse Wed Jul 25 10:23:41 2012 +0100
+++ b/tools/libxl/check-xl-disk-parse Wed Jul 25 11:14:05 2012 +0100
@@ -107,4 +107,38 @@ disk: {
EOF
one 0 backendtype=phy,vdev=xvdb,access=w,target=/dev/vg/guest-volume
+expected <<EOF
+disk: {
+ "backend_domid": 0,
+ "pdev_path": "",
+ "vdev": "hdc",
+ "backend": "unknown",
+ "format": "empty",
+ "script": null,
+ "removable": 1,
+ "readwrite": 0,
+ "is_cdrom": 1
+}
+
+EOF
+one 0 ,,hdc:cdrom,r
+one 0 vdev=hdc,access=r,devtype=cdrom,target+one 0 ,empty,hdc:cdrom,r
+
+expected <<EOF
+disk: {
+ "backend_domid": 0,
+ "pdev_path": null,
+ "vdev": "hdc",
+ "backend": "unknown",
+ "format": "empty",
+ "script": null,
+ "removable": 1,
+ "readwrite": 0,
+ "is_cdrom": 1
+}
+
+EOF
+one 0 vdev=hdc,access=r,devtype=cdrom,format=empty
+
complete
diff -r f0249f4b34e3 -r 576aeedaca77 tools/libxl/libxlu_disk.c
--- a/tools/libxl/libxlu_disk.c Wed Jul 25 10:23:41 2012 +0100
+++ b/tools/libxl/libxlu_disk.c Wed Jul 25 11:14:05 2012 +0100
@@ -76,6 +76,8 @@ int xlu_disk_parse(XLU_Config *cfg,
if (disk->is_cdrom) {
disk->removable = 1;
disk->readwrite = 0;
+ if (!disk->pdev_path || !strcmp(disk->pdev_path, ""))
+ disk->format = LIBXL_DISK_FORMAT_EMPTY;
}
if (!disk->vdev) {
diff -r f0249f4b34e3 -r 576aeedaca77 tools/libxl/libxlu_disk_l.c
--- a/tools/libxl/libxlu_disk_l.c Wed Jul 25 10:23:41 2012 +0100
+++ b/tools/libxl/libxlu_disk_l.c Wed Jul 25 11:14:05 2012 +0100
@@ -841,11 +841,12 @@ static void setaccess(DiskParseContext *
/* Sets ->format from the string. IDL should provide something for this. */
static void setformat(DiskParseContext *dpc, const char *str) {
- if (!strcmp(str,"") ||
- !strcmp(str,"raw")) DSET(dpc,format,FORMAT,str,RAW);
+ if (!strcmp(str,"")) DSET(dpc,format,FORMAT,str,RAW);
+ else if (!strcmp(str,"raw")) DSET(dpc,format,FORMAT,str,RAW);
else if (!strcmp(str,"qcow")) DSET(dpc,format,FORMAT,str,QCOW);
else if (!strcmp(str,"qcow2"))
DSET(dpc,format,FORMAT,str,QCOW2);
else if (!strcmp(str,"vhd")) DSET(dpc,format,FORMAT,str,VHD);
+ else if (!strcmp(str,"empty"))
DSET(dpc,format,FORMAT,str,EMPTY);
else xlu__disk_err(dpc,str,"unknown value for format");
}
@@ -860,7 +861,7 @@ static void setbackendtype(DiskParseCont
#define DEPRECATE(usewhatinstead) /* not currently reported */
-#line 864 "libxlu_disk_l.c"
+#line 865 "libxlu_disk_l.c"
#define INITIAL 0
#define LEXERR 1
@@ -1096,12 +1097,12 @@ YY_DECL
register int yy_act;
struct yyguts_t * yyg = (struct yyguts_t*)yyscanner;
-#line 126 "libxlu_disk_l.l"
+#line 127 "libxlu_disk_l.l"
/*----- the scanner rules which do the parsing -----*/
-#line 1105 "libxlu_disk_l.c"
+#line 1106 "libxlu_disk_l.c"
if ( !yyg->yy_init )
{
@@ -1222,72 +1223,72 @@ do_action: /* This label is used only to
case 1:
/* rule 1 can match eol */
YY_RULE_SETUP
-#line 130 "libxlu_disk_l.l"
+#line 131 "libxlu_disk_l.l"
{ /* ignore whitespace before parameters */ }
YY_BREAK
/* ordinary parameters setting enums or strings */
case 2:
/* rule 2 can match eol */
YY_RULE_SETUP
-#line 134 "libxlu_disk_l.l"
+#line 135 "libxlu_disk_l.l"
{ STRIP('',''); setformat(DPC, FROMEQUALS); }
YY_BREAK
case 3:
YY_RULE_SETUP
-#line 136 "libxlu_disk_l.l"
+#line 137 "libxlu_disk_l.l"
{ DPC->disk->is_cdrom = 1; }
YY_BREAK
case 4:
YY_RULE_SETUP
-#line 137 "libxlu_disk_l.l"
+#line 138 "libxlu_disk_l.l"
{ DPC->disk->is_cdrom = 1; }
YY_BREAK
case 5:
YY_RULE_SETUP
-#line 138 "libxlu_disk_l.l"
+#line 139 "libxlu_disk_l.l"
{ DPC->disk->is_cdrom = 0; }
YY_BREAK
case 6:
/* rule 6 can match eol */
YY_RULE_SETUP
-#line 139 "libxlu_disk_l.l"
+#line 140 "libxlu_disk_l.l"
{ xlu__disk_err(DPC,yytext,"unknown value for type"); }
YY_BREAK
case 7:
/* rule 7 can match eol */
YY_RULE_SETUP
-#line 141 "libxlu_disk_l.l"
+#line 142 "libxlu_disk_l.l"
{ STRIP('',''); setaccess(DPC, FROMEQUALS); }
YY_BREAK
case 8:
/* rule 8 can match eol */
YY_RULE_SETUP
-#line 142 "libxlu_disk_l.l"
+#line 143 "libxlu_disk_l.l"
{ STRIP('',''); setbackendtype(DPC,FROMEQUALS); }
YY_BREAK
case 9:
/* rule 9 can match eol */
YY_RULE_SETUP
-#line 144 "libxlu_disk_l.l"
+#line 145 "libxlu_disk_l.l"
{ STRIP('',''); SAVESTRING("vdev", vdev, FROMEQUALS);
}
YY_BREAK
case 10:
/* rule 10 can match eol */
YY_RULE_SETUP
-#line 145 "libxlu_disk_l.l"
+#line 146 "libxlu_disk_l.l"
{ STRIP('',''); SAVESTRING("script", script,
FROMEQUALS); }
YY_BREAK
/* the target magic parameter, eats the rest of the string */
case 11:
YY_RULE_SETUP
-#line 149 "libxlu_disk_l.l"
+#line 150 "libxlu_disk_l.l"
{ STRIP('',''); SAVESTRING("target", pdev_path,
FROMEQUALS); }
YY_BREAK
/* unknown parameters */
case 12:
/* rule 12 can match eol */
YY_RULE_SETUP
-#line 153 "libxlu_disk_l.l"
+#line 154 "libxlu_disk_l.l"
{ xlu__disk_err(DPC,yytext,"unknown parameter"); }
YY_BREAK
/* deprecated prefixes */
@@ -1295,7 +1296,7 @@ YY_RULE_SETUP
* matched the whole string, so these patterns take precedence */
case 13:
YY_RULE_SETUP
-#line 160 "libxlu_disk_l.l"
+#line 161 "libxlu_disk_l.l"
{
STRIP('':'');
DPC->had_depr_prefix=1; DEPRECATE("use
`[format=]...,''");
@@ -1304,7 +1305,7 @@ YY_RULE_SETUP
YY_BREAK
case 14:
YY_RULE_SETUP
-#line 166 "libxlu_disk_l.l"
+#line 167 "libxlu_disk_l.l"
{
STRIP('':'');
DPC->had_depr_prefix=1; DEPRECATE("use
`script=...''");
@@ -1316,12 +1317,12 @@ case 15:
yyg->yy_c_buf_p = yy_cp = yy_bp + 8;
YY_DO_BEFORE_ACTION; /* set up yytext again */
YY_RULE_SETUP
-#line 172 "libxlu_disk_l.l"
+#line 173 "libxlu_disk_l.l"
{ DPC->had_depr_prefix=1; DEPRECATE(0); }
YY_BREAK
case 16:
YY_RULE_SETUP
-#line 173 "libxlu_disk_l.l"
+#line 174 "libxlu_disk_l.l"
{ DPC->had_depr_prefix=1; DEPRECATE(0); }
YY_BREAK
case 17:
@@ -1329,7 +1330,7 @@ case 17:
yyg->yy_c_buf_p = yy_cp = yy_bp + 4;
YY_DO_BEFORE_ACTION; /* set up yytext again */
YY_RULE_SETUP
-#line 174 "libxlu_disk_l.l"
+#line 175 "libxlu_disk_l.l"
{ DPC->had_depr_prefix=1; DEPRECATE(0); }
YY_BREAK
case 18:
@@ -1337,7 +1338,7 @@ case 18:
yyg->yy_c_buf_p = yy_cp = yy_bp + 6;
YY_DO_BEFORE_ACTION; /* set up yytext again */
YY_RULE_SETUP
-#line 175 "libxlu_disk_l.l"
+#line 176 "libxlu_disk_l.l"
{ DPC->had_depr_prefix=1; DEPRECATE(0); }
YY_BREAK
case 19:
@@ -1345,7 +1346,7 @@ case 19:
yyg->yy_c_buf_p = yy_cp = yy_bp + 5;
YY_DO_BEFORE_ACTION; /* set up yytext again */
YY_RULE_SETUP
-#line 176 "libxlu_disk_l.l"
+#line 177 "libxlu_disk_l.l"
{ DPC->had_depr_prefix=1; DEPRECATE(0); }
YY_BREAK
case 20:
@@ -1353,13 +1354,13 @@ case 20:
yyg->yy_c_buf_p = yy_cp = yy_bp + 4;
YY_DO_BEFORE_ACTION; /* set up yytext again */
YY_RULE_SETUP
-#line 177 "libxlu_disk_l.l"
+#line 178 "libxlu_disk_l.l"
{ DPC->had_depr_prefix=1; DEPRECATE(0); }
YY_BREAK
case 21:
/* rule 21 can match eol */
YY_RULE_SETUP
-#line 179 "libxlu_disk_l.l"
+#line 180 "libxlu_disk_l.l"
{
xlu__disk_err(DPC,yytext,"unknown deprecated disk prefix");
return 0;
@@ -1369,7 +1370,7 @@ YY_RULE_SETUP
case 22:
/* rule 22 can match eol */
YY_RULE_SETUP
-#line 186 "libxlu_disk_l.l"
+#line 187 "libxlu_disk_l.l"
{
char *colon;
STRIP('','');
@@ -1406,7 +1407,7 @@ YY_RULE_SETUP
YY_BREAK
case 23:
YY_RULE_SETUP
-#line 220 "libxlu_disk_l.l"
+#line 221 "libxlu_disk_l.l"
{
BEGIN(LEXERR);
yymore();
@@ -1414,17 +1415,17 @@ YY_RULE_SETUP
YY_BREAK
case 24:
YY_RULE_SETUP
-#line 224 "libxlu_disk_l.l"
+#line 225 "libxlu_disk_l.l"
{
xlu__disk_err(DPC,yytext,"bad disk syntax"); return 0;
}
YY_BREAK
case 25:
YY_RULE_SETUP
-#line 227 "libxlu_disk_l.l"
+#line 228 "libxlu_disk_l.l"
YY_FATAL_ERROR( "flex scanner jammed" );
YY_BREAK
-#line 1428 "libxlu_disk_l.c"
+#line 1429 "libxlu_disk_l.c"
case YY_STATE_EOF(INITIAL):
case YY_STATE_EOF(LEXERR):
yyterminate();
@@ -2516,12 +2517,4 @@ void xlu__disk_yyfree (void * ptr , yysc
#define YYTABLES_NAME "yytables"
-#line 227 "libxlu_disk_l.l"
-
-/*
- * Local variables:
- * mode: C
- * c-basic-offset: 4
- * indent-tabs-mode: nil
- * End:
- */
+#line 228 "libxlu_disk_l.l"
diff -r f0249f4b34e3 -r 576aeedaca77 tools/libxl/libxlu_disk_l.h
--- a/tools/libxl/libxlu_disk_l.h Wed Jul 25 10:23:41 2012 +0100
+++ b/tools/libxl/libxlu_disk_l.h Wed Jul 25 11:14:05 2012 +0100
@@ -340,16 +340,8 @@ extern int xlu__disk_yylex (yyscan_t yys
#undef YY_DECL
#endif
-#line 227 "libxlu_disk_l.l"
+#line 228 "libxlu_disk_l.l"
#line 346 "libxlu_disk_l.h"
#undef xlu__disk_yyIN_HEADER
#endif /* xlu__disk_yyHEADER_H */
-
-/*
- * Local variables:
- * mode: C
- * c-basic-offset: 4
- * indent-tabs-mode: nil
- * End:
- */
diff -r f0249f4b34e3 -r 576aeedaca77 tools/libxl/libxlu_disk_l.l
--- a/tools/libxl/libxlu_disk_l.l Wed Jul 25 10:23:41 2012 +0100
+++ b/tools/libxl/libxlu_disk_l.l Wed Jul 25 11:14:05 2012 +0100
@@ -92,11 +92,12 @@ static void setaccess(DiskParseContext *
/* Sets ->format from the string. IDL should provide something for this. */
static void setformat(DiskParseContext *dpc, const char *str) {
- if (!strcmp(str,"") ||
- !strcmp(str,"raw")) DSET(dpc,format,FORMAT,str,RAW);
+ if (!strcmp(str,"")) DSET(dpc,format,FORMAT,str,RAW);
+ else if (!strcmp(str,"raw")) DSET(dpc,format,FORMAT,str,RAW);
else if (!strcmp(str,"qcow")) DSET(dpc,format,FORMAT,str,QCOW);
else if (!strcmp(str,"qcow2"))
DSET(dpc,format,FORMAT,str,QCOW2);
else if (!strcmp(str,"vhd")) DSET(dpc,format,FORMAT,str,VHD);
+ else if (!strcmp(str,"empty"))
DSET(dpc,format,FORMAT,str,EMPTY);
else xlu__disk_err(dpc,str,"unknown value for format");
}
diff -r f0249f4b34e3 -r 576aeedaca77 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c Wed Jul 25 10:23:41 2012 +0100
+++ b/tools/libxl/xl_cmdimpl.c Wed Jul 25 11:14:05 2012 +0100
@@ -2211,7 +2211,8 @@ static void cd_insert(const char *dom, c
find_domain(dom);
- if (asprintf(&buf, "%s,%s:cdrom,r", phys ? phys :
"", virtdev) < 0) {
+ if (asprintf(&buf,
"vdev=%s,access=r,devtype=cdrom,target=%s",
+ virtdev, phys ? phys : "") < 0) {
fprintf(stderr, "out of memory\n");
return;
}
@@ -2220,9 +2221,19 @@ static void cd_insert(const char *dom, c
disk.backend_domid = 0;
- libxl_cdrom_insert(ctx, domid, &disk, NULL);
-
- libxl_device_disk_dispose(&disk);
+ if (dryrun_only) {
+ char *json = libxl_device_disk_to_json(ctx, &disk);
+ printf("disk: %s\n", json);
+ free(json);
+ if (ferror(stdout) || fflush(stdout)) { perror("stdout");
exit(-1); }
+ return;
+ } else {
+ if (libxl_cdrom_insert(ctx, domid, &disk, NULL)) {
+ fprintf(stderr, "libxl_cdrom_insert failed.\n");
+
+ libxl_device_disk_dispose(&disk);
+ }
+ }
free(buf);
}
diff -r f0249f4b34e3 -r 576aeedaca77 tools/libxl/xl_cmdtable.c
--- a/tools/libxl/xl_cmdtable.c Wed Jul 25 10:23:41 2012 +0100
+++ b/tools/libxl/xl_cmdtable.c Wed Jul 25 11:14:05 2012 +0100
@@ -174,12 +174,12 @@ struct cmd_spec cmd_table[] = {
"- for internal use only",
},
{ "cd-insert",
- &main_cd_insert, 0, 1,
+ &main_cd_insert, 1, 1,
"Insert a cdrom into a guest''s cd drive",
"<Domain> <VirtualDevice> <type:path>",
},
{ "cd-eject",
- &main_cd_eject, 0, 1,
+ &main_cd_eject, 1, 1,
"Eject a cdrom from a guest''s cd drive",
"<Domain> <VirtualDevice>",
},
On Wed, 2012-07-25 at 11:15 +0100, Ian Campbell wrote:> xend accepts '',hdc:cdrom,r''[0] as an empty CDROM drive however this is > not consistent with the xl syntax in > docs/misc/xl-disk-configuration.txt which > requires '',,hdc:cdrom,r'' (the additional positional paramter is the > format). > I''m not sure if/how this can be fixed. Note that xend does not accept > '',,hdc:cdrom,r''This can be fixed with the following hack to the parser. Effectively it says that when target is given as a positional parameter and is blank then format is implicitly "empty" and must be omitted. I''m really not sure about the hunk in xlu_disk_parse. It doesn''t seem to be necessary (the tests work both with and without it) but perhaps it is wise since when we parse the target positional param we don''t yet know if the device will have :cdrom specified. I''m inclined towards doing this (and folding it into the original patch) for compatibility with xm, thoughts? 8<----------------------------- diff -r 576aeedaca77 docs/misc/xl-disk-configuration.txt --- a/docs/misc/xl-disk-configuration.txt Wed Jul 25 11:14:05 2012 +0100 +++ b/docs/misc/xl-disk-configuration.txt Wed Jul 25 11:31:53 2012 +0100 @@ -91,6 +91,9 @@ Supported values: raw, qcow, qcow2, Deprecated values: None Default value: raw +When "target" has been provided as a positional parameter and is empty +(the empty CDROM case referred to above) then this field is implicitly +"empty" and must not be specified as a positional parameter. vdev ---- diff -r 576aeedaca77 tools/libxl/check-xl-disk-parse --- a/tools/libxl/check-xl-disk-parse Wed Jul 25 11:14:05 2012 +0100 +++ b/tools/libxl/check-xl-disk-parse Wed Jul 25 11:31:53 2012 +0100 @@ -121,9 +121,12 @@ disk: { } EOF -one 0 ,,hdc:cdrom,r +one 0 ,hdc:cdrom,r one 0 vdev=hdc,access=r,devtype=cdrom,target-one 0 ,empty,hdc:cdrom,r + +expected <<EOF +EOF +one 255 ,,hdc:cdrom,r expected <<EOF disk: { diff -r 576aeedaca77 tools/libxl/libxlu_disk.c --- a/tools/libxl/libxlu_disk.c Wed Jul 25 11:14:05 2012 +0100 +++ b/tools/libxl/libxlu_disk.c Wed Jul 25 11:31:53 2012 +0100 @@ -78,6 +78,8 @@ int xlu_disk_parse(XLU_Config *cfg, disk->readwrite = 0; if (!disk->pdev_path || !strcmp(disk->pdev_path, "")) disk->format = LIBXL_DISK_FORMAT_EMPTY; + } else if (disk->format == LIBXL_DISK_FORMAT_EMPTY) { + disk->format = LIBXL_DISK_FORMAT_RAW; } if (!disk->vdev) { diff -r 576aeedaca77 tools/libxl/libxlu_disk_l.c diff -r 576aeedaca77 tools/libxl/libxlu_disk_l.h diff -r 576aeedaca77 tools/libxl/libxlu_disk_l.l --- a/tools/libxl/libxlu_disk_l.l Wed Jul 25 11:14:05 2012 +0100 +++ b/tools/libxl/libxlu_disk_l.l Wed Jul 25 11:31:53 2012 +0100 @@ -192,6 +192,8 @@ phy:/.* { DPC->had_depr_prefix=1; DEPRE /* previous errors may just lead to subsequent ones */ } else if (!DPC->disk->pdev_path) { SAVESTRING("target", pdev_path, yytext); + if (!strcmp(DPC->disk->pdev_path, "")) + DPC->disk->format = LIBXL_DISK_FORMAT_EMPTY; } else if (!DPC->had_depr_prefix && DPC->disk->format == LIBXL_DISK_FORMAT_UNKNOWN) { setformat(DPC,yytext);
Ian Campbell writes ("Re: [Xen-devel] [PATCH] xl: support empty CDROM
devices"):> On Wed, 2012-07-25 at 11:15 +0100, Ian Campbell wrote:
> > xend accepts '',hdc:cdrom,r''[0] as an empty CDROM
drive however this is
> > not consistent with the xl syntax in
> > docs/misc/xl-disk-configuration.txt which
> > requires '',,hdc:cdrom,r'' (the additional positional
paramter is the
> > format).
> > I''m not sure if/how this can be fixed. Note that xend does
not accept
> > '',,hdc:cdrom,r''
>
> This can be fixed with the following hack to the parser. Effectively it
> says that when target is given as a positional parameter and is blank
> then format is implicitly "empty" and must be omitted.
I don''t like this because it will live with us for evermore. We will
never be able to get rid of the weird wrinkle in the positional
syntax.
Maybe it would be better to do something ad hoc. I have an idea which
I will try to implement...
Ian.
On Wed, 2012-07-25 at 11:47 +0100, Ian Jackson wrote:> Ian Campbell writes ("Re: [Xen-devel] [PATCH] xl: support empty CDROM devices"): > > On Wed, 2012-07-25 at 11:15 +0100, Ian Campbell wrote: > > > xend accepts '',hdc:cdrom,r''[0] as an empty CDROM drive however this is > > > not consistent with the xl syntax in > > > docs/misc/xl-disk-configuration.txt which > > > requires '',,hdc:cdrom,r'' (the additional positional paramter is the > > > format). > > > I''m not sure if/how this can be fixed. Note that xend does not accept > > > '',,hdc:cdrom,r'' > > > > This can be fixed with the following hack to the parser. Effectively it > > says that when target is given as a positional parameter and is blank > > then format is implicitly "empty" and must be omitted. > > I don''t like this because it will live with us for evermore. We will > never be able to get rid of the weird wrinkle in the positional > syntax.I thought the positional syntax was deprecated, but rereading the doc I see it is not and it is only the "[<format>:][<target>],<vdev>[:<devtype>],<access>" syntax which is deprecated. The problematic ",hdc:cdrom,r" is actually a special case of this deprecated syntax.> Maybe it would be better to do something ad hoc. I have an idea which > I will try to implement...OK. Ian.
Ian Jackson
2012-Jul-25 15:21 UTC
[PATCH 0/2] xl: handle empty xl cdrom devices in xend syntax
Ian Campbell writes ("Re: [Xen-devel] [PATCH] xl: support empty CDROM
devices"):> On Wed, 2012-07-25 at 11:47 +0100, Ian Jackson wrote:
> > Maybe it would be better to do something ad hoc. I have an idea which
> > I will try to implement...
>
> OK.
This seems to have worked. Here is the result. This patch
- contains a copy of some your empty disk format tests
- when applied to -unstable only passes the tests if your
+ if (!disk->pdev_path || !strcmp(disk->pdev_path, ""))
+ disk->format = LIBXL_DISK_FORMAT_EMPTY;
is included and the references to the `empty'' disks are
commented out from check-xl-disk-parse.
Did you want me to merge it properly with yours ?
Ian.
Ian Jackson
2012-Jul-25 15:22 UTC
[PATCH 1/2] xl: disk parsing preparation for empty cdrom devices
Prepare the ground for parsing the xend empty cdrom syntax, by
separating out some non-functional changes as this pre-patch:
* Clarify the disk syntax documentation wording to refer to deprecated
syntaxes too.
* Make DPC in libxlu_disk_l.l useable in the helper functions as well
as in lexer rules, by providing two definitions, each in force in
the appropriate parts of the file.
* Break the <vdev>[:<devtype>] parsing out into a helper function,
`vdev_and_devtype''.
No functional change.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
docs/misc/xl-disk-configuration.txt | 11 ++--
tools/libxl/libxlu_disk_l.c | 112 ++++++++++++++++++-----------------
tools/libxl/libxlu_disk_l.h | 10 +---
tools/libxl/libxlu_disk_l.l | 44 +++++++++-----
4 files changed, 93 insertions(+), 84 deletions(-)
diff --git a/docs/misc/xl-disk-configuration.txt
b/docs/misc/xl-disk-configuration.txt
index 3bf20bf..740b8f6 100644
--- a/docs/misc/xl-disk-configuration.txt
+++ b/docs/misc/xl-disk-configuration.txt
@@ -164,16 +164,17 @@ information to be interpreted by
/etc/xen/scripts/block-<script>.
-=================================-DEPRECATED PARAMETERS AND PREFIXES
-=================================+===========================================+DEPRECATED
PARAMETERS, PREFIXES AND SYNTAXES
+===========================================
-Deprecated values are acceptable and are intended work compatibly with
+Deprecated forms are acceptable and are intended work compatibly with
xend and xl from xen 4.1. In future they may print a warning.
Support for deprecated parameters and syntaxes are likely to be
dropped in future versions of xl.
-There is also support for a deprecated old syntax for <diskspec>:
+
+There is support for a deprecated old syntax for <diskspec>:
[<format>:][<target>],<vdev>[:<devtype>],<access>
(deprecated)
diff --git a/tools/libxl/libxlu_disk_l.c b/tools/libxl/libxlu_disk_l.c
index 2f0f283..aac816a 100644
--- a/tools/libxl/libxlu_disk_l.c
+++ b/tools/libxl/libxlu_disk_l.c
@@ -794,8 +794,6 @@ void xlu__disk_yyset_column(int column_no, yyscan_t
yyscanner);
* and particularly to avoid repeating boilerplate values such as
* DPC->disk, yytext, etc. */
-#define DPC ((DiskParseContext*)yyextra)
-
/* Sets an enum, checking it hasn''t already been set to a different
value */
#define DSET(dpc,member,enumname,str,valname) do{ \
if (dpc->disk->member != LIBXL_DISK_##enumname##_UNKNOWN && \
@@ -828,6 +826,8 @@ static void savestring(DiskParseContext *dpc, const char
*what_respecified,
*update = strdup(value);
}
+#define DPC dpc /* our convention in lexer helper functions */
+
/* Sets ->readwrite from the string. This ought to be an enum, perhaps. */
static void setaccess(DiskParseContext *dpc, const char *str) {
if (!strcmp(str, "r") || !strcmp(str, "ro")) {
@@ -859,8 +859,32 @@ static void setbackendtype(DiskParseContext *dpc, const
char *str) {
#define DEPRECATE(usewhatinstead) /* not currently reported */
+/* Handles a vdev positional parameter which includes a devtype. */
+static int vdev_and_devtype(DiskParseContext *dpc, char *str) {
+ /* returns 1 if it was <vdev>:<devtype>, 0 (doing nothing)
otherwise */
+ char *colon = strrchr(str, '':'');
+ if (!colon)
+ return 0;
+
+ DEPRECATE("use `devtype=...''");
+ *colon++ = 0;
+ SAVESTRING("vdev", vdev, str);
+
+ if (!strcmp(colon,"cdrom")) {
+ DPC->disk->is_cdrom = 1;
+ } else if (!strcmp(colon,"disk")) {
+ DPC->disk->is_cdrom = 0;
+ } else {
+ xlu__disk_err(DPC,colon,"unknown deprecated type");
+ }
+ return 1;
+}
+
+#undef DPC /* needs to be defined differently the actual lexer */
+#define DPC ((DiskParseContext*)yyextra)
-#line 864 "libxlu_disk_l.c"
+
+#line 888 "libxlu_disk_l.c"
#define INITIAL 0
#define LEXERR 1
@@ -1096,12 +1120,12 @@ YY_DECL
register int yy_act;
struct yyguts_t * yyg = (struct yyguts_t*)yyscanner;
-#line 126 "libxlu_disk_l.l"
+#line 150 "libxlu_disk_l.l"
/*----- the scanner rules which do the parsing -----*/
-#line 1105 "libxlu_disk_l.c"
+#line 1129 "libxlu_disk_l.c"
if ( !yyg->yy_init )
{
@@ -1222,72 +1246,72 @@ do_action: /* This label is used only to access EOF
actions. */
case 1:
/* rule 1 can match eol */
YY_RULE_SETUP
-#line 130 "libxlu_disk_l.l"
+#line 154 "libxlu_disk_l.l"
{ /* ignore whitespace before parameters */ }
YY_BREAK
/* ordinary parameters setting enums or strings */
case 2:
/* rule 2 can match eol */
YY_RULE_SETUP
-#line 134 "libxlu_disk_l.l"
+#line 158 "libxlu_disk_l.l"
{ STRIP('',''); setformat(DPC, FROMEQUALS); }
YY_BREAK
case 3:
YY_RULE_SETUP
-#line 136 "libxlu_disk_l.l"
+#line 160 "libxlu_disk_l.l"
{ DPC->disk->is_cdrom = 1; }
YY_BREAK
case 4:
YY_RULE_SETUP
-#line 137 "libxlu_disk_l.l"
+#line 161 "libxlu_disk_l.l"
{ DPC->disk->is_cdrom = 1; }
YY_BREAK
case 5:
YY_RULE_SETUP
-#line 138 "libxlu_disk_l.l"
+#line 162 "libxlu_disk_l.l"
{ DPC->disk->is_cdrom = 0; }
YY_BREAK
case 6:
/* rule 6 can match eol */
YY_RULE_SETUP
-#line 139 "libxlu_disk_l.l"
+#line 163 "libxlu_disk_l.l"
{ xlu__disk_err(DPC,yytext,"unknown value for type"); }
YY_BREAK
case 7:
/* rule 7 can match eol */
YY_RULE_SETUP
-#line 141 "libxlu_disk_l.l"
+#line 165 "libxlu_disk_l.l"
{ STRIP('',''); setaccess(DPC, FROMEQUALS); }
YY_BREAK
case 8:
/* rule 8 can match eol */
YY_RULE_SETUP
-#line 142 "libxlu_disk_l.l"
+#line 166 "libxlu_disk_l.l"
{ STRIP('',''); setbackendtype(DPC,FROMEQUALS); }
YY_BREAK
case 9:
/* rule 9 can match eol */
YY_RULE_SETUP
-#line 144 "libxlu_disk_l.l"
+#line 168 "libxlu_disk_l.l"
{ STRIP('',''); SAVESTRING("vdev", vdev, FROMEQUALS);
}
YY_BREAK
case 10:
/* rule 10 can match eol */
YY_RULE_SETUP
-#line 145 "libxlu_disk_l.l"
+#line 169 "libxlu_disk_l.l"
{ STRIP('',''); SAVESTRING("script", script,
FROMEQUALS); }
YY_BREAK
/* the target magic parameter, eats the rest of the string */
case 11:
YY_RULE_SETUP
-#line 149 "libxlu_disk_l.l"
+#line 173 "libxlu_disk_l.l"
{ STRIP('',''); SAVESTRING("target", pdev_path,
FROMEQUALS); }
YY_BREAK
/* unknown parameters */
case 12:
/* rule 12 can match eol */
YY_RULE_SETUP
-#line 153 "libxlu_disk_l.l"
+#line 177 "libxlu_disk_l.l"
{ xlu__disk_err(DPC,yytext,"unknown parameter"); }
YY_BREAK
/* deprecated prefixes */
@@ -1295,7 +1319,7 @@ YY_RULE_SETUP
* matched the whole string, so these patterns take precedence */
case 13:
YY_RULE_SETUP
-#line 160 "libxlu_disk_l.l"
+#line 184 "libxlu_disk_l.l"
{
STRIP('':'');
DPC->had_depr_prefix=1; DEPRECATE("use
`[format=]...,''");
@@ -1304,7 +1328,7 @@ YY_RULE_SETUP
YY_BREAK
case 14:
YY_RULE_SETUP
-#line 166 "libxlu_disk_l.l"
+#line 190 "libxlu_disk_l.l"
{
STRIP('':'');
DPC->had_depr_prefix=1; DEPRECATE("use
`script=...''");
@@ -1316,12 +1340,12 @@ case 15:
yyg->yy_c_buf_p = yy_cp = yy_bp + 8;
YY_DO_BEFORE_ACTION; /* set up yytext again */
YY_RULE_SETUP
-#line 172 "libxlu_disk_l.l"
+#line 196 "libxlu_disk_l.l"
{ DPC->had_depr_prefix=1; DEPRECATE(0); }
YY_BREAK
case 16:
YY_RULE_SETUP
-#line 173 "libxlu_disk_l.l"
+#line 197 "libxlu_disk_l.l"
{ DPC->had_depr_prefix=1; DEPRECATE(0); }
YY_BREAK
case 17:
@@ -1329,7 +1353,7 @@ case 17:
yyg->yy_c_buf_p = yy_cp = yy_bp + 4;
YY_DO_BEFORE_ACTION; /* set up yytext again */
YY_RULE_SETUP
-#line 174 "libxlu_disk_l.l"
+#line 198 "libxlu_disk_l.l"
{ DPC->had_depr_prefix=1; DEPRECATE(0); }
YY_BREAK
case 18:
@@ -1337,7 +1361,7 @@ case 18:
yyg->yy_c_buf_p = yy_cp = yy_bp + 6;
YY_DO_BEFORE_ACTION; /* set up yytext again */
YY_RULE_SETUP
-#line 175 "libxlu_disk_l.l"
+#line 199 "libxlu_disk_l.l"
{ DPC->had_depr_prefix=1; DEPRECATE(0); }
YY_BREAK
case 19:
@@ -1345,7 +1369,7 @@ case 19:
yyg->yy_c_buf_p = yy_cp = yy_bp + 5;
YY_DO_BEFORE_ACTION; /* set up yytext again */
YY_RULE_SETUP
-#line 176 "libxlu_disk_l.l"
+#line 200 "libxlu_disk_l.l"
{ DPC->had_depr_prefix=1; DEPRECATE(0); }
YY_BREAK
case 20:
@@ -1353,13 +1377,13 @@ case 20:
yyg->yy_c_buf_p = yy_cp = yy_bp + 4;
YY_DO_BEFORE_ACTION; /* set up yytext again */
YY_RULE_SETUP
-#line 177 "libxlu_disk_l.l"
+#line 201 "libxlu_disk_l.l"
{ DPC->had_depr_prefix=1; DEPRECATE(0); }
YY_BREAK
case 21:
/* rule 21 can match eol */
YY_RULE_SETUP
-#line 179 "libxlu_disk_l.l"
+#line 203 "libxlu_disk_l.l"
{
xlu__disk_err(DPC,yytext,"unknown deprecated disk prefix");
return 0;
@@ -1369,9 +1393,8 @@ YY_RULE_SETUP
case 22:
/* rule 22 can match eol */
YY_RULE_SETUP
-#line 186 "libxlu_disk_l.l"
+#line 210 "libxlu_disk_l.l"
{
- char *colon;
STRIP('','');
if (DPC->err) {
@@ -1382,19 +1405,8 @@ YY_RULE_SETUP
DPC->disk->format == LIBXL_DISK_FORMAT_UNKNOWN) {
setformat(DPC,yytext);
} else if (!DPC->disk->vdev) {
- colon = strrchr(yytext, '':'');
- if (colon) {
- DEPRECATE("use `devtype=...''");
- *colon++ = 0;
- if (!strcmp(colon,"cdrom")) {
- DPC->disk->is_cdrom = 1;
- } else if (!strcmp(colon,"disk")) {
- DPC->disk->is_cdrom = 0;
- } else {
- xlu__disk_err(DPC,colon,"unknown deprecated type");
- }
- }
- SAVESTRING("vdev", vdev, yytext);
+ if (!vdev_and_devtype(DPC,yytext))
+ SAVESTRING("vdev", vdev, yytext);
} else if (!DPC->access_set) {
DPC->access_set = 1;
setaccess(DPC,yytext);
@@ -1406,7 +1418,7 @@ YY_RULE_SETUP
YY_BREAK
case 23:
YY_RULE_SETUP
-#line 220 "libxlu_disk_l.l"
+#line 232 "libxlu_disk_l.l"
{
BEGIN(LEXERR);
yymore();
@@ -1414,17 +1426,17 @@ YY_RULE_SETUP
YY_BREAK
case 24:
YY_RULE_SETUP
-#line 224 "libxlu_disk_l.l"
+#line 236 "libxlu_disk_l.l"
{
xlu__disk_err(DPC,yytext,"bad disk syntax"); return 0;
}
YY_BREAK
case 25:
YY_RULE_SETUP
-#line 227 "libxlu_disk_l.l"
+#line 239 "libxlu_disk_l.l"
YY_FATAL_ERROR( "flex scanner jammed" );
YY_BREAK
-#line 1428 "libxlu_disk_l.c"
+#line 1440 "libxlu_disk_l.c"
case YY_STATE_EOF(INITIAL):
case YY_STATE_EOF(LEXERR):
yyterminate();
@@ -2516,12 +2528,4 @@ void xlu__disk_yyfree (void * ptr , yyscan_t yyscanner)
#define YYTABLES_NAME "yytables"
-#line 227 "libxlu_disk_l.l"
-
-/*
- * Local variables:
- * mode: C
- * c-basic-offset: 4
- * indent-tabs-mode: nil
- * End:
- */
+#line 239 "libxlu_disk_l.l"
diff --git a/tools/libxl/libxlu_disk_l.h b/tools/libxl/libxlu_disk_l.h
index a3fbd72..72b82cb 100644
--- a/tools/libxl/libxlu_disk_l.h
+++ b/tools/libxl/libxlu_disk_l.h
@@ -340,16 +340,8 @@ extern int xlu__disk_yylex (yyscan_t yyscanner);
#undef YY_DECL
#endif
-#line 227 "libxlu_disk_l.l"
+#line 239 "libxlu_disk_l.l"
#line 346 "libxlu_disk_l.h"
#undef xlu__disk_yyIN_HEADER
#endif /* xlu__disk_yyHEADER_H */
-
-/*
- * Local variables:
- * mode: C
- * c-basic-offset: 4
- * indent-tabs-mode: nil
- * End:
- */
diff --git a/tools/libxl/libxlu_disk_l.l b/tools/libxl/libxlu_disk_l.l
index a3e7180..9fbc1ad 100644
--- a/tools/libxl/libxlu_disk_l.l
+++ b/tools/libxl/libxlu_disk_l.l
@@ -45,8 +45,6 @@ void xlu__disk_yyset_column(int column_no, yyscan_t
yyscanner);
* and particularly to avoid repeating boilerplate values such as
* DPC->disk, yytext, etc. */
-#define DPC ((DiskParseContext*)yyextra)
-
/* Sets an enum, checking it hasn''t already been set to a different
value */
#define DSET(dpc,member,enumname,str,valname) do{ \
if (dpc->disk->member != LIBXL_DISK_##enumname##_UNKNOWN && \
@@ -79,6 +77,8 @@ static void savestring(DiskParseContext *dpc, const char
*what_respecified,
*update = strdup(value);
}
+#define DPC dpc /* our convention in lexer helper functions */
+
/* Sets ->readwrite from the string. This ought to be an enum, perhaps. */
static void setaccess(DiskParseContext *dpc, const char *str) {
if (!strcmp(str, "r") || !strcmp(str, "ro")) {
@@ -110,6 +110,30 @@ static void setbackendtype(DiskParseContext *dpc, const
char *str) {
#define DEPRECATE(usewhatinstead) /* not currently reported */
+/* Handles a vdev positional parameter which includes a devtype. */
+static int vdev_and_devtype(DiskParseContext *dpc, char *str) {
+ /* returns 1 if it was <vdev>:<devtype>, 0 (doing nothing)
otherwise */
+ char *colon = strrchr(str, '':'');
+ if (!colon)
+ return 0;
+
+ DEPRECATE("use `devtype=...''");
+ *colon++ = 0;
+ SAVESTRING("vdev", vdev, str);
+
+ if (!strcmp(colon,"cdrom")) {
+ DPC->disk->is_cdrom = 1;
+ } else if (!strcmp(colon,"disk")) {
+ DPC->disk->is_cdrom = 0;
+ } else {
+ xlu__disk_err(DPC,colon,"unknown deprecated type");
+ }
+ return 1;
+}
+
+#undef DPC /* needs to be defined differently the actual lexer */
+#define DPC ((DiskParseContext*)yyextra)
+
%}
%option warn
@@ -184,7 +208,6 @@ phy:/.* { DPC->had_depr_prefix=1; DEPRECATE(0); }
/* positional parameters */
[^=,]*,|[^=,]+,? {
- char *colon;
STRIP('','');
if (DPC->err) {
@@ -195,19 +218,8 @@ phy:/.* { DPC->had_depr_prefix=1; DEPRECATE(0); }
DPC->disk->format == LIBXL_DISK_FORMAT_UNKNOWN) {
setformat(DPC,yytext);
} else if (!DPC->disk->vdev) {
- colon = strrchr(yytext, '':'');
- if (colon) {
- DEPRECATE("use `devtype=...''");
- *colon++ = 0;
- if (!strcmp(colon,"cdrom")) {
- DPC->disk->is_cdrom = 1;
- } else if (!strcmp(colon,"disk")) {
- DPC->disk->is_cdrom = 0;
- } else {
- xlu__disk_err(DPC,colon,"unknown deprecated type");
- }
- }
- SAVESTRING("vdev", vdev, yytext);
+ if (!vdev_and_devtype(DPC,yytext))
+ SAVESTRING("vdev", vdev, yytext);
} else if (!DPC->access_set) {
DPC->access_set = 1;
setaccess(DPC,yytext);
--
1.7.2.5
xend accepts '',hdc:cdrom,r''[0] as an empty CDROM drive.
However this
is not consistent with the existing xl syntax in
docs/misc/xl-disk-configuration.txt which requires `,,hdc:cdrom,r''
(the additional positional paramter is the format).
We fix this by spotting the case specially: when the target is empty
and the format contains a colon, reinterpret the format as
<vdev>:<devtype>.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
docs/misc/xl-disk-configuration.txt | 11 ++++++++++
tools/libxl/check-xl-disk-parse | 36 +++++++++++++++++++++++++++++++++++
tools/libxl/libxlu_disk_l.c | 16 +++++++++-----
tools/libxl/libxlu_disk_l.h | 2 +-
tools/libxl/libxlu_disk_l.l | 6 ++++-
5 files changed, 63 insertions(+), 8 deletions(-)
diff --git a/docs/misc/xl-disk-configuration.txt
b/docs/misc/xl-disk-configuration.txt
index 740b8f6..5da5e11 100644
--- a/docs/misc/xl-disk-configuration.txt
+++ b/docs/misc/xl-disk-configuration.txt
@@ -221,3 +221,14 @@ to specify several of these, for example:
"tap:aio:/some/path..."
All of these prefixes are now stripped and ignored.
+
+
+Missing format and empty target
+-------------------------------
+
+The following syntax is also supported:
+
+ ,<vdev>:<devtype>,<access> (deprecated)
+
+This is soley for compatibility with xend''s syntax for empty cdroms,
+which is (for example) ",hdc:cdrom,r".
diff --git a/tools/libxl/check-xl-disk-parse b/tools/libxl/check-xl-disk-parse
index 67805e5..ffe3613 100755
--- a/tools/libxl/check-xl-disk-parse
+++ b/tools/libxl/check-xl-disk-parse
@@ -107,4 +107,40 @@ disk: {
EOF
one 0 backendtype=phy,vdev=xvdb,access=w,target=/dev/vg/guest-volume
+expected <<EOF
+disk: {
+ "backend_domid": 0,
+ "pdev_path": "",
+ "vdev": "hdc",
+ "backend": "unknown",
+ "format": "empty",
+ "script": null,
+ "removable": 1,
+ "readwrite": 0,
+ "is_cdrom": 1
+}
+
+EOF
+one 0 devtype=cdrom,,,hdc
+one 0 ,,hdc:cdrom,r
+one 0 ,hdc:cdrom,r
+one 0 vdev=hdc,access=r,devtype=cdrom,target+one 0 ,empty,hdc:cdrom,r
+
+expected <<EOF
+disk: {
+ "backend_domid": 0,
+ "pdev_path": null,
+ "vdev": "hdc",
+ "backend": "unknown",
+ "format": "empty",
+ "script": null,
+ "removable": 1,
+ "readwrite": 0,
+ "is_cdrom": 1
+}
+
+EOF
+one 0 vdev=hdc,access=r,devtype=cdrom,format=empty
+
complete
diff --git a/tools/libxl/libxlu_disk_l.c b/tools/libxl/libxlu_disk_l.c
index aac816a..42b4c22 100644
--- a/tools/libxl/libxlu_disk_l.c
+++ b/tools/libxl/libxlu_disk_l.c
@@ -1403,7 +1403,11 @@ YY_RULE_SETUP
SAVESTRING("target", pdev_path, yytext);
} else if (!DPC->had_depr_prefix &&
DPC->disk->format == LIBXL_DISK_FORMAT_UNKNOWN) {
- setformat(DPC,yytext);
+ if (!*DPC->disk->pdev_path &&
vdev_and_devtype(DPC,yytext)) {
+ DPC->disk->format = LIBXL_DISK_FORMAT_EMPTY;
+ } else {
+ setformat(DPC,yytext);
+ }
} else if (!DPC->disk->vdev) {
if (!vdev_and_devtype(DPC,yytext))
SAVESTRING("vdev", vdev, yytext);
@@ -1418,7 +1422,7 @@ YY_RULE_SETUP
YY_BREAK
case 23:
YY_RULE_SETUP
-#line 232 "libxlu_disk_l.l"
+#line 236 "libxlu_disk_l.l"
{
BEGIN(LEXERR);
yymore();
@@ -1426,17 +1430,17 @@ YY_RULE_SETUP
YY_BREAK
case 24:
YY_RULE_SETUP
-#line 236 "libxlu_disk_l.l"
+#line 240 "libxlu_disk_l.l"
{
xlu__disk_err(DPC,yytext,"bad disk syntax"); return 0;
}
YY_BREAK
case 25:
YY_RULE_SETUP
-#line 239 "libxlu_disk_l.l"
+#line 243 "libxlu_disk_l.l"
YY_FATAL_ERROR( "flex scanner jammed" );
YY_BREAK
-#line 1440 "libxlu_disk_l.c"
+#line 1444 "libxlu_disk_l.c"
case YY_STATE_EOF(INITIAL):
case YY_STATE_EOF(LEXERR):
yyterminate();
@@ -2528,4 +2532,4 @@ void xlu__disk_yyfree (void * ptr , yyscan_t yyscanner)
#define YYTABLES_NAME "yytables"
-#line 239 "libxlu_disk_l.l"
+#line 243 "libxlu_disk_l.l"
diff --git a/tools/libxl/libxlu_disk_l.h b/tools/libxl/libxlu_disk_l.h
index 72b82cb..b0e6aa3 100644
--- a/tools/libxl/libxlu_disk_l.h
+++ b/tools/libxl/libxlu_disk_l.h
@@ -340,7 +340,7 @@ extern int xlu__disk_yylex (yyscan_t yyscanner);
#undef YY_DECL
#endif
-#line 239 "libxlu_disk_l.l"
+#line 243 "libxlu_disk_l.l"
#line 346 "libxlu_disk_l.h"
#undef xlu__disk_yyIN_HEADER
diff --git a/tools/libxl/libxlu_disk_l.l b/tools/libxl/libxlu_disk_l.l
index 9fbc1ad..508bb5a 100644
--- a/tools/libxl/libxlu_disk_l.l
+++ b/tools/libxl/libxlu_disk_l.l
@@ -216,7 +216,11 @@ phy:/.* { DPC->had_depr_prefix=1; DEPRECATE(0); }
SAVESTRING("target", pdev_path, yytext);
} else if (!DPC->had_depr_prefix &&
DPC->disk->format == LIBXL_DISK_FORMAT_UNKNOWN) {
- setformat(DPC,yytext);
+ if (!*DPC->disk->pdev_path &&
vdev_and_devtype(DPC,yytext)) {
+ DPC->disk->format = LIBXL_DISK_FORMAT_EMPTY;
+ } else {
+ setformat(DPC,yytext);
+ }
} else if (!DPC->disk->vdev) {
if (!vdev_and_devtype(DPC,yytext))
SAVESTRING("vdev", vdev, yytext);
--
1.7.2.5
Ian Campbell
2012-Jul-25 15:29 UTC
Re: [PATCH 0/2] xl: handle empty xl cdrom devices in xend syntax
On Wed, 2012-07-25 at 16:21 +0100, Ian Jackson wrote:> Ian Campbell writes ("Re: [Xen-devel] [PATCH] xl: support empty CDROM devices"): > > On Wed, 2012-07-25 at 11:47 +0100, Ian Jackson wrote: > > > Maybe it would be better to do something ad hoc. I have an idea which > > > I will try to implement... > > > > OK. > > This seems to have worked.Cool> Here is the result. This patch > - contains a copy of some your empty disk format tests > - when applied to -unstable only passes the tests if your > + if (!disk->pdev_path || !strcmp(disk->pdev_path, "")) > + disk->format = LIBXL_DISK_FORMAT_EMPTY; > is included and the references to the `empty'' disks are > commented out from check-xl-disk-parse.I''m not sure what specifically you have commented out here. so...> Did you want me to merge it properly with yours ?... I think this might be a good idea rather than have me make a mess of it. Ian
Ian Campbell
2012-Jul-25 15:31 UTC
Re: [PATCH 2/2] xl: support xend empty cdrom device syntax
On Wed, 2012-07-25 at 16:22 +0100, Ian Jackson wrote:> xend accepts '',hdc:cdrom,r''[0] as an empty CDROM drive. However this > is not consistent with the existing xl syntax in > docs/misc/xl-disk-configuration.txt which requires `,,hdc:cdrom,r'' > (the additional positional paramter is the format). > > We fix this by spotting the case specially: when the target is empty > and the format contains a colon, reinterpret the format as > <vdev>:<devtype>. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> > --- > docs/misc/xl-disk-configuration.txt | 11 ++++++++++ > tools/libxl/check-xl-disk-parse | 36 +++++++++++++++++++++++++++++++++++ > tools/libxl/libxlu_disk_l.c | 16 +++++++++----- > tools/libxl/libxlu_disk_l.h | 2 +- > tools/libxl/libxlu_disk_l.l | 6 ++++- > 5 files changed, 63 insertions(+), 8 deletions(-) > > diff --git a/docs/misc/xl-disk-configuration.txt b/docs/misc/xl-disk-configuration.txt > index 740b8f6..5da5e11 100644 > --- a/docs/misc/xl-disk-configuration.txt > +++ b/docs/misc/xl-disk-configuration.txt > @@ -221,3 +221,14 @@ to specify several of these, for example: > "tap:aio:/some/path..." > > All of these prefixes are now stripped and ignored. > + > + > +Missing format and empty target > +------------------------------- > + > +The following syntax is also supported: > + > + ,<vdev>:<devtype>,<access> (deprecated) > + > +This is soley for compatibility with xend''s syntax for empty cdroms, > +which is (for example) ",hdc:cdrom,r".Perhaps we should tighten the special case to specifically only accept ,<vdev>:cdrom,<access> ? (and actually as it happens <access> has to be "r" too) Also perhaps it would be useful to always give the mapping to the non-deprecated syntax?> diff --git a/tools/libxl/check-xl-disk-parse b/tools/libxl/check-xl-disk-parse > index 67805e5..ffe3613 100755 > --- a/tools/libxl/check-xl-disk-parse > +++ b/tools/libxl/check-xl-disk-parse > @@ -107,4 +107,40 @@ disk: { > EOF > one 0 backendtype=phy,vdev=xvdb,access=w,target=/dev/vg/guest-volume > > +expected <<EOF > +disk: { > + "backend_domid": 0, > + "pdev_path": "", > + "vdev": "hdc", > + "backend": "unknown", > + "format": "empty", > + "script": null, > + "removable": 1, > + "readwrite": 0, > + "is_cdrom": 1 > +} > + > +EOF > +one 0 devtype=cdrom,,,hdc > +one 0 ,,hdc:cdrom,r > +one 0 ,hdc:cdrom,r > +one 0 vdev=hdc,access=r,devtype=cdrom,target> +one 0 ,empty,hdc:cdrom,r > + > +expected <<EOF > +disk: { > + "backend_domid": 0, > + "pdev_path": null, > + "vdev": "hdc", > + "backend": "unknown", > + "format": "empty", > + "script": null, > + "removable": 1, > + "readwrite": 0, > + "is_cdrom": 1 > +} > + > +EOF > +one 0 vdev=hdc,access=r,devtype=cdrom,format=empty > + > complete > diff --git a/tools/libxl/libxlu_disk_l.c b/tools/libxl/libxlu_disk_l.c > index aac816a..42b4c22 100644 > --- a/tools/libxl/libxlu_disk_l.c > +++ b/tools/libxl/libxlu_disk_l.c > @@ -1403,7 +1403,11 @@ YY_RULE_SETUP > SAVESTRING("target", pdev_path, yytext); > } else if (!DPC->had_depr_prefix && > DPC->disk->format == LIBXL_DISK_FORMAT_UNKNOWN) { > - setformat(DPC,yytext); > + if (!*DPC->disk->pdev_path && vdev_and_devtype(DPC,yytext)) { > + DPC->disk->format = LIBXL_DISK_FORMAT_EMPTY; > + } else { > + setformat(DPC,yytext); > + } > } else if (!DPC->disk->vdev) { > if (!vdev_and_devtype(DPC,yytext)) > SAVESTRING("vdev", vdev, yytext); > @@ -1418,7 +1422,7 @@ YY_RULE_SETUP > YY_BREAK > case 23: > YY_RULE_SETUP > -#line 232 "libxlu_disk_l.l" > +#line 236 "libxlu_disk_l.l" > { > BEGIN(LEXERR); > yymore(); > @@ -1426,17 +1430,17 @@ YY_RULE_SETUP > YY_BREAK > case 24: > YY_RULE_SETUP > -#line 236 "libxlu_disk_l.l" > +#line 240 "libxlu_disk_l.l" > { > xlu__disk_err(DPC,yytext,"bad disk syntax"); return 0; > } > YY_BREAK > case 25: > YY_RULE_SETUP > -#line 239 "libxlu_disk_l.l" > +#line 243 "libxlu_disk_l.l" > YY_FATAL_ERROR( "flex scanner jammed" ); > YY_BREAK > -#line 1440 "libxlu_disk_l.c" > +#line 1444 "libxlu_disk_l.c" > case YY_STATE_EOF(INITIAL): > case YY_STATE_EOF(LEXERR): > yyterminate(); > @@ -2528,4 +2532,4 @@ void xlu__disk_yyfree (void * ptr , yyscan_t yyscanner) > > #define YYTABLES_NAME "yytables" > > -#line 239 "libxlu_disk_l.l" > +#line 243 "libxlu_disk_l.l" > diff --git a/tools/libxl/libxlu_disk_l.h b/tools/libxl/libxlu_disk_l.h > index 72b82cb..b0e6aa3 100644 > --- a/tools/libxl/libxlu_disk_l.h > +++ b/tools/libxl/libxlu_disk_l.h > @@ -340,7 +340,7 @@ extern int xlu__disk_yylex (yyscan_t yyscanner); > #undef YY_DECL > #endif > > -#line 239 "libxlu_disk_l.l" > +#line 243 "libxlu_disk_l.l" > > #line 346 "libxlu_disk_l.h" > #undef xlu__disk_yyIN_HEADER > diff --git a/tools/libxl/libxlu_disk_l.l b/tools/libxl/libxlu_disk_l.l > index 9fbc1ad..508bb5a 100644 > --- a/tools/libxl/libxlu_disk_l.l > +++ b/tools/libxl/libxlu_disk_l.l > @@ -216,7 +216,11 @@ phy:/.* { DPC->had_depr_prefix=1; DEPRECATE(0); } > SAVESTRING("target", pdev_path, yytext); > } else if (!DPC->had_depr_prefix && > DPC->disk->format == LIBXL_DISK_FORMAT_UNKNOWN) { > - setformat(DPC,yytext); > + if (!*DPC->disk->pdev_path && vdev_and_devtype(DPC,yytext)) { > + DPC->disk->format = LIBXL_DISK_FORMAT_EMPTY; > + } else { > + setformat(DPC,yytext); > + } > } else if (!DPC->disk->vdev) { > if (!vdev_and_devtype(DPC,yytext)) > SAVESTRING("vdev", vdev, yytext);
Ian Campbell
2012-Jul-25 15:32 UTC
Re: [PATCH 1/2] xl: disk parsing preparation for empty cdrom devices
On Wed, 2012-07-25 at 16:22 +0100, Ian Jackson wrote:> Prepare the ground for parsing the xend empty cdrom syntax, by > separating out some non-functional changes as this pre-patch: > > * Clarify the disk syntax documentation wording to refer to deprecated > syntaxes too. > > * Make DPC in libxlu_disk_l.l useable in the helper functions as well > as in lexer rules, by providing two definitions, each in force in > the appropriate parts of the file. > > * Break the <vdev>[:<devtype>] parsing out into a helper function, > `vdev_and_devtype''. > > No functional change. > > Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>Acked-by: Ian Campbell <ian.campbell@citrix.com>> --- > docs/misc/xl-disk-configuration.txt | 11 ++-- > tools/libxl/libxlu_disk_l.c | 112 ++++++++++++++++++----------------- > tools/libxl/libxlu_disk_l.h | 10 +--- > tools/libxl/libxlu_disk_l.l | 44 +++++++++----- > 4 files changed, 93 insertions(+), 84 deletions(-) > > diff --git a/docs/misc/xl-disk-configuration.txt b/docs/misc/xl-disk-configuration.txt > index 3bf20bf..740b8f6 100644 > --- a/docs/misc/xl-disk-configuration.txt > +++ b/docs/misc/xl-disk-configuration.txt > @@ -164,16 +164,17 @@ information to be interpreted by /etc/xen/scripts/block-<script>. > > > > -=================================> -DEPRECATED PARAMETERS AND PREFIXES > -=================================> +===========================================> +DEPRECATED PARAMETERS, PREFIXES AND SYNTAXES > +===========================================> > -Deprecated values are acceptable and are intended work compatibly with > +Deprecated forms are acceptable and are intended work compatibly with > xend and xl from xen 4.1. In future they may print a warning. > Support for deprecated parameters and syntaxes are likely to be > dropped in future versions of xl. > > -There is also support for a deprecated old syntax for <diskspec>: > + > +There is support for a deprecated old syntax for <diskspec>: > > [<format>:][<target>],<vdev>[:<devtype>],<access> (deprecated) > > diff --git a/tools/libxl/libxlu_disk_l.c b/tools/libxl/libxlu_disk_l.c > index 2f0f283..aac816a 100644 > --- a/tools/libxl/libxlu_disk_l.c > +++ b/tools/libxl/libxlu_disk_l.c > @@ -794,8 +794,6 @@ void xlu__disk_yyset_column(int column_no, yyscan_t yyscanner); > * and particularly to avoid repeating boilerplate values such as > * DPC->disk, yytext, etc. */ > > -#define DPC ((DiskParseContext*)yyextra) > - > /* Sets an enum, checking it hasn''t already been set to a different value */ > #define DSET(dpc,member,enumname,str,valname) do{ \ > if (dpc->disk->member != LIBXL_DISK_##enumname##_UNKNOWN && \ > @@ -828,6 +826,8 @@ static void savestring(DiskParseContext *dpc, const char *what_respecified, > *update = strdup(value); > } > > +#define DPC dpc /* our convention in lexer helper functions */ > + > /* Sets ->readwrite from the string. This ought to be an enum, perhaps. */ > static void setaccess(DiskParseContext *dpc, const char *str) { > if (!strcmp(str, "r") || !strcmp(str, "ro")) { > @@ -859,8 +859,32 @@ static void setbackendtype(DiskParseContext *dpc, const char *str) { > > #define DEPRECATE(usewhatinstead) /* not currently reported */ > > +/* Handles a vdev positional parameter which includes a devtype. */ > +static int vdev_and_devtype(DiskParseContext *dpc, char *str) { > + /* returns 1 if it was <vdev>:<devtype>, 0 (doing nothing) otherwise */ > + char *colon = strrchr(str, '':''); > + if (!colon) > + return 0; > + > + DEPRECATE("use `devtype=...''"); > + *colon++ = 0; > + SAVESTRING("vdev", vdev, str); > + > + if (!strcmp(colon,"cdrom")) { > + DPC->disk->is_cdrom = 1; > + } else if (!strcmp(colon,"disk")) { > + DPC->disk->is_cdrom = 0; > + } else { > + xlu__disk_err(DPC,colon,"unknown deprecated type"); > + } > + return 1; > +} > + > +#undef DPC /* needs to be defined differently the actual lexer */ > +#define DPC ((DiskParseContext*)yyextra) > > -#line 864 "libxlu_disk_l.c" > + > +#line 888 "libxlu_disk_l.c" > > #define INITIAL 0 > #define LEXERR 1 > @@ -1096,12 +1120,12 @@ YY_DECL > register int yy_act; > struct yyguts_t * yyg = (struct yyguts_t*)yyscanner; > > -#line 126 "libxlu_disk_l.l" > +#line 150 "libxlu_disk_l.l" > > > /*----- the scanner rules which do the parsing -----*/ > > -#line 1105 "libxlu_disk_l.c" > +#line 1129 "libxlu_disk_l.c" > > if ( !yyg->yy_init ) > { > @@ -1222,72 +1246,72 @@ do_action: /* This label is used only to access EOF actions. */ > case 1: > /* rule 1 can match eol */ > YY_RULE_SETUP > -#line 130 "libxlu_disk_l.l" > +#line 154 "libxlu_disk_l.l" > { /* ignore whitespace before parameters */ } > YY_BREAK > /* ordinary parameters setting enums or strings */ > case 2: > /* rule 2 can match eol */ > YY_RULE_SETUP > -#line 134 "libxlu_disk_l.l" > +#line 158 "libxlu_disk_l.l" > { STRIP('',''); setformat(DPC, FROMEQUALS); } > YY_BREAK > case 3: > YY_RULE_SETUP > -#line 136 "libxlu_disk_l.l" > +#line 160 "libxlu_disk_l.l" > { DPC->disk->is_cdrom = 1; } > YY_BREAK > case 4: > YY_RULE_SETUP > -#line 137 "libxlu_disk_l.l" > +#line 161 "libxlu_disk_l.l" > { DPC->disk->is_cdrom = 1; } > YY_BREAK > case 5: > YY_RULE_SETUP > -#line 138 "libxlu_disk_l.l" > +#line 162 "libxlu_disk_l.l" > { DPC->disk->is_cdrom = 0; } > YY_BREAK > case 6: > /* rule 6 can match eol */ > YY_RULE_SETUP > -#line 139 "libxlu_disk_l.l" > +#line 163 "libxlu_disk_l.l" > { xlu__disk_err(DPC,yytext,"unknown value for type"); } > YY_BREAK > case 7: > /* rule 7 can match eol */ > YY_RULE_SETUP > -#line 141 "libxlu_disk_l.l" > +#line 165 "libxlu_disk_l.l" > { STRIP('',''); setaccess(DPC, FROMEQUALS); } > YY_BREAK > case 8: > /* rule 8 can match eol */ > YY_RULE_SETUP > -#line 142 "libxlu_disk_l.l" > +#line 166 "libxlu_disk_l.l" > { STRIP('',''); setbackendtype(DPC,FROMEQUALS); } > YY_BREAK > case 9: > /* rule 9 can match eol */ > YY_RULE_SETUP > -#line 144 "libxlu_disk_l.l" > +#line 168 "libxlu_disk_l.l" > { STRIP('',''); SAVESTRING("vdev", vdev, FROMEQUALS); } > YY_BREAK > case 10: > /* rule 10 can match eol */ > YY_RULE_SETUP > -#line 145 "libxlu_disk_l.l" > +#line 169 "libxlu_disk_l.l" > { STRIP('',''); SAVESTRING("script", script, FROMEQUALS); } > YY_BREAK > /* the target magic parameter, eats the rest of the string */ > case 11: > YY_RULE_SETUP > -#line 149 "libxlu_disk_l.l" > +#line 173 "libxlu_disk_l.l" > { STRIP('',''); SAVESTRING("target", pdev_path, FROMEQUALS); } > YY_BREAK > /* unknown parameters */ > case 12: > /* rule 12 can match eol */ > YY_RULE_SETUP > -#line 153 "libxlu_disk_l.l" > +#line 177 "libxlu_disk_l.l" > { xlu__disk_err(DPC,yytext,"unknown parameter"); } > YY_BREAK > /* deprecated prefixes */ > @@ -1295,7 +1319,7 @@ YY_RULE_SETUP > * matched the whole string, so these patterns take precedence */ > case 13: > YY_RULE_SETUP > -#line 160 "libxlu_disk_l.l" > +#line 184 "libxlu_disk_l.l" > { > STRIP('':''); > DPC->had_depr_prefix=1; DEPRECATE("use `[format=]...,''"); > @@ -1304,7 +1328,7 @@ YY_RULE_SETUP > YY_BREAK > case 14: > YY_RULE_SETUP > -#line 166 "libxlu_disk_l.l" > +#line 190 "libxlu_disk_l.l" > { > STRIP('':''); > DPC->had_depr_prefix=1; DEPRECATE("use `script=...''"); > @@ -1316,12 +1340,12 @@ case 15: > yyg->yy_c_buf_p = yy_cp = yy_bp + 8; > YY_DO_BEFORE_ACTION; /* set up yytext again */ > YY_RULE_SETUP > -#line 172 "libxlu_disk_l.l" > +#line 196 "libxlu_disk_l.l" > { DPC->had_depr_prefix=1; DEPRECATE(0); } > YY_BREAK > case 16: > YY_RULE_SETUP > -#line 173 "libxlu_disk_l.l" > +#line 197 "libxlu_disk_l.l" > { DPC->had_depr_prefix=1; DEPRECATE(0); } > YY_BREAK > case 17: > @@ -1329,7 +1353,7 @@ case 17: > yyg->yy_c_buf_p = yy_cp = yy_bp + 4; > YY_DO_BEFORE_ACTION; /* set up yytext again */ > YY_RULE_SETUP > -#line 174 "libxlu_disk_l.l" > +#line 198 "libxlu_disk_l.l" > { DPC->had_depr_prefix=1; DEPRECATE(0); } > YY_BREAK > case 18: > @@ -1337,7 +1361,7 @@ case 18: > yyg->yy_c_buf_p = yy_cp = yy_bp + 6; > YY_DO_BEFORE_ACTION; /* set up yytext again */ > YY_RULE_SETUP > -#line 175 "libxlu_disk_l.l" > +#line 199 "libxlu_disk_l.l" > { DPC->had_depr_prefix=1; DEPRECATE(0); } > YY_BREAK > case 19: > @@ -1345,7 +1369,7 @@ case 19: > yyg->yy_c_buf_p = yy_cp = yy_bp + 5; > YY_DO_BEFORE_ACTION; /* set up yytext again */ > YY_RULE_SETUP > -#line 176 "libxlu_disk_l.l" > +#line 200 "libxlu_disk_l.l" > { DPC->had_depr_prefix=1; DEPRECATE(0); } > YY_BREAK > case 20: > @@ -1353,13 +1377,13 @@ case 20: > yyg->yy_c_buf_p = yy_cp = yy_bp + 4; > YY_DO_BEFORE_ACTION; /* set up yytext again */ > YY_RULE_SETUP > -#line 177 "libxlu_disk_l.l" > +#line 201 "libxlu_disk_l.l" > { DPC->had_depr_prefix=1; DEPRECATE(0); } > YY_BREAK > case 21: > /* rule 21 can match eol */ > YY_RULE_SETUP > -#line 179 "libxlu_disk_l.l" > +#line 203 "libxlu_disk_l.l" > { > xlu__disk_err(DPC,yytext,"unknown deprecated disk prefix"); > return 0; > @@ -1369,9 +1393,8 @@ YY_RULE_SETUP > case 22: > /* rule 22 can match eol */ > YY_RULE_SETUP > -#line 186 "libxlu_disk_l.l" > +#line 210 "libxlu_disk_l.l" > { > - char *colon; > STRIP('',''); > > if (DPC->err) { > @@ -1382,19 +1405,8 @@ YY_RULE_SETUP > DPC->disk->format == LIBXL_DISK_FORMAT_UNKNOWN) { > setformat(DPC,yytext); > } else if (!DPC->disk->vdev) { > - colon = strrchr(yytext, '':''); > - if (colon) { > - DEPRECATE("use `devtype=...''"); > - *colon++ = 0; > - if (!strcmp(colon,"cdrom")) { > - DPC->disk->is_cdrom = 1; > - } else if (!strcmp(colon,"disk")) { > - DPC->disk->is_cdrom = 0; > - } else { > - xlu__disk_err(DPC,colon,"unknown deprecated type"); > - } > - } > - SAVESTRING("vdev", vdev, yytext); > + if (!vdev_and_devtype(DPC,yytext)) > + SAVESTRING("vdev", vdev, yytext); > } else if (!DPC->access_set) { > DPC->access_set = 1; > setaccess(DPC,yytext); > @@ -1406,7 +1418,7 @@ YY_RULE_SETUP > YY_BREAK > case 23: > YY_RULE_SETUP > -#line 220 "libxlu_disk_l.l" > +#line 232 "libxlu_disk_l.l" > { > BEGIN(LEXERR); > yymore(); > @@ -1414,17 +1426,17 @@ YY_RULE_SETUP > YY_BREAK > case 24: > YY_RULE_SETUP > -#line 224 "libxlu_disk_l.l" > +#line 236 "libxlu_disk_l.l" > { > xlu__disk_err(DPC,yytext,"bad disk syntax"); return 0; > } > YY_BREAK > case 25: > YY_RULE_SETUP > -#line 227 "libxlu_disk_l.l" > +#line 239 "libxlu_disk_l.l" > YY_FATAL_ERROR( "flex scanner jammed" ); > YY_BREAK > -#line 1428 "libxlu_disk_l.c" > +#line 1440 "libxlu_disk_l.c" > case YY_STATE_EOF(INITIAL): > case YY_STATE_EOF(LEXERR): > yyterminate(); > @@ -2516,12 +2528,4 @@ void xlu__disk_yyfree (void * ptr , yyscan_t yyscanner) > > #define YYTABLES_NAME "yytables" > > -#line 227 "libxlu_disk_l.l" > - > -/* > - * Local variables: > - * mode: C > - * c-basic-offset: 4 > - * indent-tabs-mode: nil > - * End: > - */ > +#line 239 "libxlu_disk_l.l" > diff --git a/tools/libxl/libxlu_disk_l.h b/tools/libxl/libxlu_disk_l.h > index a3fbd72..72b82cb 100644 > --- a/tools/libxl/libxlu_disk_l.h > +++ b/tools/libxl/libxlu_disk_l.h > @@ -340,16 +340,8 @@ extern int xlu__disk_yylex (yyscan_t yyscanner); > #undef YY_DECL > #endif > > -#line 227 "libxlu_disk_l.l" > +#line 239 "libxlu_disk_l.l" > > #line 346 "libxlu_disk_l.h" > #undef xlu__disk_yyIN_HEADER > #endif /* xlu__disk_yyHEADER_H */ > - > -/* > - * Local variables: > - * mode: C > - * c-basic-offset: 4 > - * indent-tabs-mode: nil > - * End: > - */ > diff --git a/tools/libxl/libxlu_disk_l.l b/tools/libxl/libxlu_disk_l.l > index a3e7180..9fbc1ad 100644 > --- a/tools/libxl/libxlu_disk_l.l > +++ b/tools/libxl/libxlu_disk_l.l > @@ -45,8 +45,6 @@ void xlu__disk_yyset_column(int column_no, yyscan_t yyscanner); > * and particularly to avoid repeating boilerplate values such as > * DPC->disk, yytext, etc. */ > > -#define DPC ((DiskParseContext*)yyextra) > - > /* Sets an enum, checking it hasn''t already been set to a different value */ > #define DSET(dpc,member,enumname,str,valname) do{ \ > if (dpc->disk->member != LIBXL_DISK_##enumname##_UNKNOWN && \ > @@ -79,6 +77,8 @@ static void savestring(DiskParseContext *dpc, const char *what_respecified, > *update = strdup(value); > } > > +#define DPC dpc /* our convention in lexer helper functions */ > + > /* Sets ->readwrite from the string. This ought to be an enum, perhaps. */ > static void setaccess(DiskParseContext *dpc, const char *str) { > if (!strcmp(str, "r") || !strcmp(str, "ro")) { > @@ -110,6 +110,30 @@ static void setbackendtype(DiskParseContext *dpc, const char *str) { > > #define DEPRECATE(usewhatinstead) /* not currently reported */ > > +/* Handles a vdev positional parameter which includes a devtype. */ > +static int vdev_and_devtype(DiskParseContext *dpc, char *str) { > + /* returns 1 if it was <vdev>:<devtype>, 0 (doing nothing) otherwise */ > + char *colon = strrchr(str, '':''); > + if (!colon) > + return 0; > + > + DEPRECATE("use `devtype=...''"); > + *colon++ = 0; > + SAVESTRING("vdev", vdev, str); > + > + if (!strcmp(colon,"cdrom")) { > + DPC->disk->is_cdrom = 1; > + } else if (!strcmp(colon,"disk")) { > + DPC->disk->is_cdrom = 0; > + } else { > + xlu__disk_err(DPC,colon,"unknown deprecated type"); > + } > + return 1; > +} > + > +#undef DPC /* needs to be defined differently the actual lexer */ > +#define DPC ((DiskParseContext*)yyextra) > + > %} > > %option warn > @@ -184,7 +208,6 @@ phy:/.* { DPC->had_depr_prefix=1; DEPRECATE(0); } > /* positional parameters */ > > [^=,]*,|[^=,]+,? { > - char *colon; > STRIP('',''); > > if (DPC->err) { > @@ -195,19 +218,8 @@ phy:/.* { DPC->had_depr_prefix=1; DEPRECATE(0); } > DPC->disk->format == LIBXL_DISK_FORMAT_UNKNOWN) { > setformat(DPC,yytext); > } else if (!DPC->disk->vdev) { > - colon = strrchr(yytext, '':''); > - if (colon) { > - DEPRECATE("use `devtype=...''"); > - *colon++ = 0; > - if (!strcmp(colon,"cdrom")) { > - DPC->disk->is_cdrom = 1; > - } else if (!strcmp(colon,"disk")) { > - DPC->disk->is_cdrom = 0; > - } else { > - xlu__disk_err(DPC,colon,"unknown deprecated type"); > - } > - } > - SAVESTRING("vdev", vdev, yytext); > + if (!vdev_and_devtype(DPC,yytext)) > + SAVESTRING("vdev", vdev, yytext); > } else if (!DPC->access_set) { > DPC->access_set = 1; > setaccess(DPC,yytext); > -- > 1.7.2.5 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Ian Campbell
2012-Jul-25 15:33 UTC
Re: [PATCH 2/2] xl: support xend empty cdrom device syntax
> > +Missing format and empty target > > +------------------------------- > > + > > +The following syntax is also supported: > > + > > + ,<vdev>:<devtype>,<access> (deprecated) > > + > > +This is soley for compatibility with xend''s syntax for empty cdroms, > > +which is (for example) ",hdc:cdrom,r". > > Perhaps we should tighten the special case to specifically only > accept ,<vdev>:cdrom,<access> ? (and actually as it happens <access> has > to be "r" too)Although this would undoubtedly complexify vdev_and_devtype so feel free to ignore this suggestion...
Ian Jackson
2012-Jul-25 15:37 UTC
Re: [PATCH 2/2] xl: support xend empty cdrom device syntax
Ian Campbell writes ("Re: [PATCH 2/2] xl: support xend empty cdrom device
syntax"):> On Wed, 2012-07-25 at 16:22 +0100, Ian Jackson wrote:
> > +This is soley for compatibility with xend''s syntax for empty
cdroms,
> > +which is (for example) ",hdc:cdrom,r".
>
> Perhaps we should tighten the special case to specifically only
> accept ,<vdev>:cdrom,<access> ? (and actually as it happens
<access> has
> to be "r" too)
I don''t think that''s necessary. We don''t intend to
introduce any
formats containing `:'' so no ambiguity is introduced.
> Also perhaps it would be useful to always give the mapping to the
> non-deprecated syntax?
You mean to write the non-deprecated syntax in the document ?
Ian.
Ian Campbell
2012-Jul-25 15:41 UTC
Re: [PATCH 2/2] xl: support xend empty cdrom device syntax
On Wed, 2012-07-25 at 16:37 +0100, Ian Jackson wrote:> Ian Campbell writes ("Re: [PATCH 2/2] xl: support xend empty cdrom device syntax"): > > On Wed, 2012-07-25 at 16:22 +0100, Ian Jackson wrote: > > > +This is soley for compatibility with xend''s syntax for empty cdroms, > > > +which is (for example) ",hdc:cdrom,r". > > > > Perhaps we should tighten the special case to specifically only > > accept ,<vdev>:cdrom,<access> ? (and actually as it happens <access> has > > to be "r" too) > > I don''t think that''s necessary. We don''t intend to introduce any > formats containing `:'' so no ambiguity is introduced. > > > Also perhaps it would be useful to always give the mapping to the > > non-deprecated syntax? > > You mean to write the non-deprecated syntax in the document ?e.g.: The following syntax is also supported: ,<vdev>:<devtype>,<access> (deprecated) This is soley for compatibility with xend''s syntax for empty cdroms, which is (for example) ",hdc:cdrom,r". +The non-deprecated syntax is: + + ,,<vdev>:<devtype>,<access> or whatever the most preferable non-deprecated version is. Basically it gives a no-brainer way to update your syntax. BTW my MUA''s spell checker thinks you meant solely. Ian.
On Wed, 2012-07-25 at 11:15 +0100, Ian Campbell wrote:> # HG changeset patch > # User Ian Campbell <ian.campbell@citrix.com> > # Date 1343211245 -3600 > # Node ID 576aeedaca777236db8760ac5ca17b30f1c5354d > # Parent f0249f4b34e3ca3a770a09c688f9c1d1c523f5e7 > xl: support empty CDROM devicesIt seems like I forgot to qrefresh before sending this. I supect that given your updates these differences are now irrelevant but FTR the incremental bit which was omitted because of this was: diff -r 097bf63027e0 docs/misc/xl-disk-configuration.txt --- a/docs/misc/xl-disk-configuration.txt Wed Jul 25 11:21:55 2012 +0100 +++ b/docs/misc/xl-disk-configuration.txt Wed Jul 25 17:00:00 2012 +0100 @@ -91,8 +91,9 @@ Supported values: raw, qcow, qcow2, Deprecated values: None Default value: raw -Format cannot be specified as a positional parameter when target is -not provided (the empty CDROM case) +When "target" has been provided as a positional parameter and is empty +(the empty CDROM case referred to above) then this field is implicitly +"empty" and must not be specified as a positional parameter. vdev ---- diff -r 097bf63027e0 tools/libxl/check-xl-disk-parse --- a/tools/libxl/check-xl-disk-parse Wed Jul 25 11:21:55 2012 +0100 +++ b/tools/libxl/check-xl-disk-parse Wed Jul 25 17:00:00 2012 +0100 @@ -125,6 +125,10 @@ one 0 ,hdc:cdrom,r one 0 vdev=hdc,access=r,devtype=cdrom,target expected <<EOF +EOF +one 255 ,,hdc:cdrom,r + +expected <<EOF disk: { "backend_domid": 0, "pdev_path": null, diff -r 097bf63027e0 tools/libxl/libxlu_disk.c --- a/tools/libxl/libxlu_disk.c Wed Jul 25 11:21:55 2012 +0100 +++ b/tools/libxl/libxlu_disk.c Wed Jul 25 17:00:00 2012 +0100 @@ -78,6 +78,8 @@ int xlu_disk_parse(XLU_Config *cfg, disk->readwrite = 0; if (!disk->pdev_path || !strcmp(disk->pdev_path, "")) disk->format = LIBXL_DISK_FORMAT_EMPTY; + } else if (disk->format == LIBXL_DISK_FORMAT_EMPTY) { + disk->format = LIBXL_DISK_FORMAT_RAW; } if (!disk->vdev) { diff -r 097bf63027e0 tools/libxl/libxlu_disk_l.c --- a/tools/libxl/libxlu_disk_l.c Wed Jul 25 11:21:55 2012 +0100 +++ b/tools/libxl/libxlu_disk_l.c Wed Jul 25 17:00:00 2012 +0100 @@ -1379,8 +1379,8 @@ YY_RULE_SETUP /* previous errors may just lead to subsequent ones */ } else if (!DPC->disk->pdev_path) { SAVESTRING("target", pdev_path, yytext); - if (!strcmp(DPC->disk->pdev_path, "")) - DPC->disk->format = LIBXL_DISK_FORMAT_EMPTY; + if (!strcmp(DPC->disk->pdev_path, "")) + DPC->disk->format = LIBXL_DISK_FORMAT_EMPTY; } else if (!DPC->had_depr_prefix && DPC->disk->format == LIBXL_DISK_FORMAT_UNKNOWN) { setformat(DPC,yytext); diff -r 097bf63027e0 tools/libxl/libxlu_disk_l.l --- a/tools/libxl/libxlu_disk_l.l Wed Jul 25 11:21:55 2012 +0100 +++ b/tools/libxl/libxlu_disk_l.l Wed Jul 25 17:00:00 2012 +0100 @@ -192,8 +192,8 @@ phy:/.* { DPC->had_depr_prefix=1; DEPRE /* previous errors may just lead to subsequent ones */ } else if (!DPC->disk->pdev_path) { SAVESTRING("target", pdev_path, yytext); - if (!strcmp(DPC->disk->pdev_path, "")) - DPC->disk->format = LIBXL_DISK_FORMAT_EMPTY; + if (!strcmp(DPC->disk->pdev_path, "")) + DPC->disk->format = LIBXL_DISK_FORMAT_EMPTY; } else if (!DPC->had_depr_prefix && DPC->disk->format == LIBXL_DISK_FORMAT_UNKNOWN) { setformat(DPC,yytext);
Ian Campbell writes ("Re: [Xen-devel] [PATCH] xl: support empty CDROM
devices"):> On Wed, 2012-07-25 at 11:15 +0100, Ian Campbell wrote:
> > xl: support empty CDROM devices
>
> It seems like I forgot to qrefresh before sending this.
>
> I supect that given your updates these differences are now irrelevant
> but FTR the incremental bit which was omitted because of this was:
Thanks.
> diff -r 097bf63027e0 docs/misc/xl-disk-configuration.txt
> --- a/docs/misc/xl-disk-configuration.txt Wed Jul 25 11:21:55 2012 +0100
> +++ b/docs/misc/xl-disk-configuration.txt Wed Jul 25 17:00:00 2012 +0100
> @@ -91,8 +91,9 @@ Supported values: raw, qcow, qcow2,
> Deprecated values: None
> Default value: raw
>
> -Format cannot be specified as a positional parameter when target is
> -not provided (the empty CDROM case)
> +When "target" has been provided as a positional parameter and is
empty
> +(the empty CDROM case referred to above) then this field is implicitly
> +"empty" and must not be specified as a positional parameter.
This is no longer true in my patch.
> expected <<EOF
> +EOF
> +one 255 ,,hdc:cdrom,r
> +
> +expected <<EOF
I have this already.
> diff -r 097bf63027e0 tools/libxl/libxlu_disk.c
> --- a/tools/libxl/libxlu_disk.c Wed Jul 25 11:21:55 2012 +0100
> +++ b/tools/libxl/libxlu_disk.c Wed Jul 25 17:00:00 2012 +0100
> @@ -78,6 +78,8 @@ int xlu_disk_parse(XLU_Config *cfg,
> disk->readwrite = 0;
> if (!disk->pdev_path || !strcmp(disk->pdev_path,
""))
> disk->format = LIBXL_DISK_FORMAT_EMPTY;
> + } else if (disk->format == LIBXL_DISK_FORMAT_EMPTY) {
> + disk->format = LIBXL_DISK_FORMAT_RAW;
> }
This is rather odd. It appears to turn empty non-cdroms into RAW. Is
that actually correct ? It doesn''t seem likely to me that it is. I
think my new arrangements don''t generate empty non-cdroms unless the
user explicitly specifies `empty'' as the format or uses the xend
compatibility syntax and explicitly specifies `:disk''.
Ian.
On Wed, 2012-07-25 at 17:06 +0100, Ian Jackson wrote:> Ian Campbell writes ("Re: [Xen-devel] [PATCH] xl: support empty CDROM devices"): > > On Wed, 2012-07-25 at 11:15 +0100, Ian Campbell wrote: > > > xl: support empty CDROM devices > > > > It seems like I forgot to qrefresh before sending this. > > > > I supect that given your updates these differences are now irrelevant > > but FTR the incremental bit which was omitted because of this was: > > Thanks. > > > diff -r 097bf63027e0 docs/misc/xl-disk-configuration.txt > > --- a/docs/misc/xl-disk-configuration.txt Wed Jul 25 11:21:55 2012 +0100 > > +++ b/docs/misc/xl-disk-configuration.txt Wed Jul 25 17:00:00 2012 +0100 > > @@ -91,8 +91,9 @@ Supported values: raw, qcow, qcow2, > > Deprecated values: None > > Default value: raw > > > > -Format cannot be specified as a positional parameter when target is > > -not provided (the empty CDROM case) > > +When "target" has been provided as a positional parameter and is empty > > +(the empty CDROM case referred to above) then this field is implicitly > > +"empty" and must not be specified as a positional parameter. > > This is no longer true in my patch. > > > expected <<EOF > > +EOF > > +one 255 ,,hdc:cdrom,r > > + > > +expected <<EOF > > I have this already. > > > diff -r 097bf63027e0 tools/libxl/libxlu_disk.c > > --- a/tools/libxl/libxlu_disk.c Wed Jul 25 11:21:55 2012 +0100 > > +++ b/tools/libxl/libxlu_disk.c Wed Jul 25 17:00:00 2012 +0100 > > @@ -78,6 +78,8 @@ int xlu_disk_parse(XLU_Config *cfg, > > disk->readwrite = 0; > > if (!disk->pdev_path || !strcmp(disk->pdev_path, "")) > > disk->format = LIBXL_DISK_FORMAT_EMPTY; > > + } else if (disk->format == LIBXL_DISK_FORMAT_EMPTY) { > > + disk->format = LIBXL_DISK_FORMAT_RAW; > > } > > This is rather odd. It appears to turn empty non-cdroms into RAW. Is > that actually correct ? It doesn''t seem likely to me that it is. I > think my new arrangements don''t generate empty non-cdroms unless the > user explicitly specifies `empty'' as the format or uses the xend > compatibility syntax and explicitly specifies `:disk''.I think empty is meaningless for anything except cdroms. This was here because in my version the parser didn''t know it had a cdrom at the point where it had to decide to make the device empty so I fixed it up here. I think you are right that your version doesn''t require it.> > Ian.
Ian Campbell writes ("Re: [Xen-devel] [PATCH] xl: support empty CDROM
devices"):> On Wed, 2012-07-25 at 17:06 +0100, Ian Jackson wrote:
> > Ian Campbell writes ("Re: [Xen-devel] [PATCH] xl: support empty
CDROM devices"):
> > > diff -r 097bf63027e0 tools/libxl/libxlu_disk.c
> > > --- a/tools/libxl/libxlu_disk.c Wed Jul 25 11:21:55 2012 +0100
> > > +++ b/tools/libxl/libxlu_disk.c Wed Jul 25 17:00:00 2012 +0100
> > > @@ -78,6 +78,8 @@ int xlu_disk_parse(XLU_Config *cfg,
> > > disk->readwrite = 0;
> > > if (!disk->pdev_path || !strcmp(disk->pdev_path,
""))
> > > disk->format = LIBXL_DISK_FORMAT_EMPTY;
> > > + } else if (disk->format == LIBXL_DISK_FORMAT_EMPTY) {
> > > + disk->format = LIBXL_DISK_FORMAT_RAW;
> > > }
> >
> > This is rather odd. It appears to turn empty non-cdroms into RAW. Is
> > that actually correct ? It doesn''t seem likely to me that it
is. I
> > think my new arrangements don''t generate empty non-cdroms
unless the
> > user explicitly specifies `empty'' as the format or uses the
xend
> > compatibility syntax and explicitly specifies `:disk''.
>
> I think empty is meaningless for anything except cdroms. This was here
> because in my version the parser didn''t know it had a cdrom at the
point
> where it had to decide to make the device empty so I fixed it up here. I
> think you are right that your version doesn''t require it.
Right. I think the later code in libxl will reject empty non-cdroms
so we are fine without this hunk.
Ian.