Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better parser #18

Merged
merged 6 commits into from May 23, 2016

Conversation

Projects
None yet
8 participants
@jonludlam
Copy link
Contributor

commented Feb 26, 2016

Let's have a parser that can handle quoted arguments. Additionally, ignore arguments that don't fit the key=value model rather than dying.

jonludlam added some commits Jan 8, 2016

Switch from re.str to astring.
Also improve the parser so that it can handle quoted strings
or escaped characters. This also skips over arguments that
aren't of the form 'a=b' rather than dying.

Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>
Update opam to add a test stanza
Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>
end
| Some c ->
Printf.printf "Something went wrong in the argv parser: Matched '%c'" c;
raise Exit

This comment has been minimized.

Copy link
@Drup

Drup Feb 26, 2016

Member

Bootvar.argv returns an error type, it would be better to do something similar.

This comment has been minimized.

Copy link
@jonludlam

jonludlam Feb 26, 2016

Author Contributor

That particular case was there just to satisfy the pattern match exhaustiveness check - if we get to that clause, Astring is broken. Having said that, there's an additional 'raise Exit' a few lines above that is reachable, so I should fix that. And if the whole thing is going to return an error type, I might as well fix this one too.

jonludlam added some commits Feb 26, 2016

Switch Parse_argv.parse to return an error type
Thanks to @Drup for the suggestion.

Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>
Add a negative test for an unparsable string
Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>
@jonludlam

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2016

I cancelled two queued travis builds on older commits, hence the red crosses above.

@talex5

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2016

With this branch, booting on Qubes failes with:

.[32;1mMirageOS booting....[0m
  start_info: 0000000000273000(VA)
nr_pages: 0x1400
  shared_inf: 0x174bf000(MA)
 pt_base: 0000000000276000(VA)
nr_pt_frames: 0x5
mfn_list: 0000000000269000(VA)
   mod_start: 0x268000(VA)
 mod_len: 4096
   flags: 0x0
cmd_line: root=/dev/mapper/dmroot ro nomodeset console=hvc0 rd_NO_PLYMOUTH 3 nopat
   stack: 0000000000247900-0000000000267900
MM: Init
  _text: 0000000000000000(VA)
 _etext: 000000000012a870(VA)
   _erodata: 0000000000165000(VA)
 _edata: 000000000020dea0(VA)
stack start: 0000000000247900(VA)
   _end: 0000000000267900(VA)
  start_pfn: 27e
max_pfn: 1400
Mapping memory range 0x400000 - 0x1400000
setting 0000000000000000-0000000000165000 readonly
skipped 1000
MM: Initialise page allocator for 286000(286000)-1400000(1400000)
MM: done
Demand map pfns at 1401000-0000002001401000.
Initialising timer interface
Initialising console ... done.
gnttab_table mapped at 0000000001401000.
Netif: add resume hook
gnttab_stubs.c: initialised mini-os gntmap
kernel args: "root=/dev/mapper/dmroot ro nomodeset console=hvc0 rd_NO_PLYMOUTH 3 nopat"
Fatal error: exception Failure("Malformed boot parameter \"ro\"")
Raised at file "src/core/lwt.ml", line 901, characters 22-23
Called from file "lib/main.ml", line 58, characters 10-20
Called from file "main.ml", line 52, characters 5-10
Mirage exiting with status 2
Do_exit called!
.[32;1mMirageOS booting....[0m
  start_info: 000000000026e000(VA)
nr_pages: 0x1400
  shared_inf: 0x1fa27000(MA)
 pt_base: 0000000000271000(VA)
nr_pt_frames: 0x5
mfn_list: 0000000000264000(VA)
   mod_start: 0x263000(VA)
 mod_len: 4096
   flags: 0x0
cmd_line: root=/dev/mapper/dmroot ro nomodeset console=hvc0 rd_NO_PLYMOUTH 3 nopat
   stack: 0000000000242b80-0000000000262b80
MM: Init
  _text: 0000000000000000(VA)
 _etext: 0000000000128be0(VA)
   _erodata: 0000000000164000(VA)
 _edata: 0000000000209120(VA)
stack start: 0000000000242b80(VA)
   _end: 0000000000262b80(VA)
  start_pfn: 279
max_pfn: 1400
Mapping memory range 0x400000 - 0x1400000
setting 0000000000000000-0000000000164000 readonly
skipped 1000
MM: Initialise page allocator for 281000(281000)-1400000(1400000)
MM: done
Demand map pfns at 1401000-0000002001401000.
Initialising timer interface
Initialising console ... done.
gnttab_table mapped at 0000000001401000.
Netif: add resume hook
gnttab_stubs.c: initialised mini-os gntmap
qubes-firewall: unknown option `--console'.
Usage: qubes-firewall [OPTION]... 
Try `qubes-firewall --help' for more information.
Ignoring malformed parameter: roIgnoring malformed parameter: nomodesetIgnoring malformed parameter: rd_NO_PLYMOUTHIgnoring malformed parameter: 3Ignoring malformed parameter: nopatMirage exiting with status 1
Do_exit called!

I guess it needs some newlines added.

How do I make this work with functoria?

@jonludlam

This comment has been minimized.

Copy link
Contributor Author

commented Feb 29, 2016

Ah yes, forgot the newline. Looks like the 'console=hvc0' got through the key=value parser and was turned into '--console hvc0'. We'd need to decide on the approaches outlined in mirage/mirage#493 to progress this I think.

Add a missing newline.
Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>
@dbuenzli

This comment has been minimized.

Copy link

commented Mar 4, 2016

What about specifying the environment as pairs of positional arguments ? It removes the need for quoting, see my comment here #6 (comment).

@dbuenzli dbuenzli referenced this pull request May 21, 2016

Closed

use astring instead of re #19

@hannesm

This comment has been minimized.

Copy link
Member

commented May 22, 2016

instead of #19, I think this should get merged... ping, anyone around?

@jonludlam

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2016

Oops, had forgotten about this. @dbuenzli: This parser is parsing the Xen command line, which is just presented as a single string - your point about the key value pairs, if I understand it correctly, effectively offloads the parsing onto bash by way of it providing your argv. In this case, there's no help available, so we still need to have a careful parser.

@hannesm

This comment has been minimized.

Copy link
Member

commented May 23, 2016

I've no clue, but shouldn't the mirage-bootvar-xen just expose the string (argv) from xenstore, and this is then passed by mirage/functoria into a cmdliner? is there any need for string processing here?

@jonludlam

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2016

There's definitely a need for some string processing, as Cmdliner needs an array of strings to process, and the Xen PV ABI only provides a single string, so something's got to do some processing. The encoding is done by whoever puts it into the xl.cfg file:

http://xenbits.xen.org/docs/unstable/man/xl.cfg.5.html#Direct-Kernel-Boot

or by whoever wrote the grub config if pygrub is being used. The equivalent code in the linux kernel is here:

http://lxr.hpcs.cs.tsukuba.ac.jp/linux/kernel/params.c#L171

We could do whatever we like here - a json fragment, binio, whatever. Copying roughly what the linux kernel does seems like a reasonable idea for now.

@hannesm

This comment has been minimized.

Copy link
Member

commented May 23, 2016

and this PR even adds some testing... is it good to go, then?

@samoht

This comment has been minimized.

Copy link
Member

commented May 23, 2016

LGTM

@jonludlam

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2016

In it goes then!

@jonludlam jonludlam merged commit 7c630a4 into mirage:master May 23, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hannesm

This comment has been minimized.

Copy link
Member

commented May 26, 2016

can we have a new release with improved parsing (and modified dependencies)? :)

@samoht

This comment has been minimized.

Copy link
Member

commented May 27, 2016

ping @MagnusS

@hannesm

This comment has been minimized.

Copy link
Member

commented Jun 9, 2016

2 weeks passed... any news on a new release? :) (also I guess the open issues should be re-evaluated)

@yomimono

This comment has been minimized.

Copy link
Member

commented Jun 9, 2016

It seems I have rights to cut a release on this repository, so I'll do so soon.

@MagnusS

This comment has been minimized.

Copy link
Member

commented Jun 9, 2016

@yomimono thanks for doing the release!

Sorry for the delay - I have not had time to look at this yet :-/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.