Re: [libvirt] [PATCH] bhyve: add support for passing stdin to loader

2018-06-03 Thread Roman Bogorodskiy 
  Fabian Freyer wrote:

> On 26 Apr 2018, at 18:38, John Ferlan wrote:
> 
> > On 04/13/2018 03:27 PM, Fabian Freyer wrote:
> >> This commit adds the  node to the domain definition,
> >> with the following semantics:
> >>
> >> To pass standard input verbatim to the bootloader, set
> >>
> >> some stdin
> >>
> >> Multiline standard input can be set using a CDATA tag:
> >>
> >> 
> >>
> >> Standard input can be read from a file as follows:
> >>
> >> 
> >
> > Not my area of expertise, but some feedback to hopefully help a bit
> > seeing as there have been no other takes for 2 weeks.
> 
> Thanks!
> 
> > Personally this format over the other format would seem to be better
> > although the attribute name would be "path" not "file".  IOW: That whole
> > CDATA thing - not sure it passes by all the censors^W reviewers
> > scrutiny.  Is there a particular reason to have this CDATA tag syntax?
> The idea here is to be able to specify several lines of standard input,
> especially for line-oriented command line interfaces, while not needing
> to add a separate file.

If there are just a line or two to be passed to loader's stdin, being
able to specify that right in the domain xml feels more convenient than
creating a file.

> > Parsing the line and making sure consumers provide a specific format is
> > probably more hassle than it's worth and I would think presents an
> > opportunity to insert things that may cause interesting errors. Any time
> > you accept something from stdin like that you open up buffer overflows
> > and buffer handling problems. Not that the same thing cannot be in the
> > file, but at least you're then passing that off to something else to
> > manage whether what's in a file is correctly formatted as you're not
> > validating the contents of the file, just that it exists.
> 
> I’m not quite sure I comprehend the difference to a file here.
> 
> >>
> >> Signed-off-by: Fabian Freyer 
> >> ---
> >>  docs/formatdomain.html.in  | 19 ++

Would be good to add some example to drvbhyve.html.in as well.

> >>  docs/schemas/domaincommon.rng  | 10 
> >>  src/bhyve/bhyve_driver.c   | 10 
> >>  src/bhyve/bhyve_parse_command.c| 70 
> >> ++
> >>  src/bhyve/bhyve_process.c  | 22 +++
> >>  src/conf/domain_conf.c | 41 +
> >>  src/conf/domain_conf.h | 11 
> >>  .../bhyveargv2xml-loader-stdin-file.args   |  9 +++
> >>  .../bhyveargv2xml-loader-stdin-file.xml| 19 ++
> >>  .../bhyveargv2xml-loader-stdin-multiline.args  | 13 
> >>  .../bhyveargv2xml-loader-stdin-multiline.xml   | 21 +++
> >>  .../bhyveargv2xml-loader-stdin-oneline.args| 11 
> >>  .../bhyveargv2xml-loader-stdin-oneline.xml | 19 ++
> >>  tests/bhyveargv2xmltest.c  |  3 +
> >>  .../bhyvexml2argv-grub-stdin-file.args |  9 +++
> >>  .../bhyvexml2argv-grub-stdin-file.devmap   |  1 +
> >>  .../bhyvexml2argv-grub-stdin-file.ldargs   |  4 ++
> >>  .../bhyvexml2argv-grub-stdin-file.xml  | 25 
> >>  .../bhyvexml2argv-grub-stdin-multiline.args|  9 +++
> >>  .../bhyvexml2argv-grub-stdin-multiline.devmap  |  1 +
> >>  .../bhyvexml2argv-grub-stdin-multiline.ldargs  |  4 ++
> >>  .../bhyvexml2argv-grub-stdin-multiline.xml | 30 ++
> >>  .../bhyvexml2argv-grub-stdin-oneline.args  |  9 +++
> >>  .../bhyvexml2argv-grub-stdin-oneline.devmap|  1 +
> >>  .../bhyvexml2argv-grub-stdin-oneline.ldargs|  4 ++
> >>  .../bhyvexml2argv-grub-stdin-oneline.xml   | 25 
> >>  tests/bhyvexml2argvtest.c  |  3 +
> >>  .../bhyvexml2xmlout-grub-stdin-file.xml| 34 +++
> >>  .../bhyvexml2xmlout-grub-stdin-multiline.xml   | 39 
> >>  .../bhyvexml2xmlout-grub-stdin-oneline.xml | 34 +++
> >>  tests/bhyvexml2xmltest.c   |  3 +
> >>  31 files changed, 513 insertions(+)
> >>  create mode 100644 
> >> tests/bhyveargv2xmldata/bhyveargv2xml-loader-stdin-file.args
> >>  create mode 100644 
> >> tests/bhyveargv2xmldata/bhyveargv2xml-loader-stdin-file.xml
> >>  create mode 100644 
> >> tests/bhyveargv2xmldata/bhyveargv2xml-loader-stdin-multiline.args
> >>  create mode 100644 
> >> tests/bhyveargv2xmldata/bhyveargv2xml-loader-stdin-multiline.xml
> >>  create mode 100644 
> >> tests/bhyveargv2xmldata/bhyveargv2xml-loader-stdin-oneline.args
> >>  create mode 100644 
> >> tests/bhyveargv2xmldata/bhyveargv2xml-loader-stdin-oneline.xml
> >>  create mode 100644 
> >> tests/bhyvexml2argvdata/bhyvexml2argv-grub-stdin-file.args
> >>  create mode 100644 
> >> tests/bhyvexml2argvdata/bhyvexml2argv-grub-stdin-file.devmap
> >>  create mode 100644 
> >>

Re: [libvirt] [PATCH] bhyve: add support for passing stdin to loader

2018-04-26 Thread Fabian Freyer 


On 26 Apr 2018, at 18:38, John Ferlan wrote:

> On 04/13/2018 03:27 PM, Fabian Freyer wrote:
>> This commit adds the  node to the domain definition,
>> with the following semantics:
>>
>> To pass standard input verbatim to the bootloader, set
>>
>> some stdin
>>
>> Multiline standard input can be set using a CDATA tag:
>>
>> 
>>
>> Standard input can be read from a file as follows:
>>
>> 
>
> Not my area of expertise, but some feedback to hopefully help a bit
> seeing as there have been no other takes for 2 weeks.

Thanks!

> Personally this format over the other format would seem to be better
> although the attribute name would be "path" not "file".  IOW: That whole
> CDATA thing - not sure it passes by all the censors^W reviewers
> scrutiny.  Is there a particular reason to have this CDATA tag syntax?
The idea here is to be able to specify several lines of standard input,
especially for line-oriented command line interfaces, while not needing
to add a separate file.

> Parsing the line and making sure consumers provide a specific format is
> probably more hassle than it's worth and I would think presents an
> opportunity to insert things that may cause interesting errors. Any time
> you accept something from stdin like that you open up buffer overflows
> and buffer handling problems. Not that the same thing cannot be in the
> file, but at least you're then passing that off to something else to
> manage whether what's in a file is correctly formatted as you're not
> validating the contents of the file, just that it exists.

I’m not quite sure I comprehend the difference to a file here.

>>
>> Signed-off-by: Fabian Freyer 
>> ---
>>  docs/formatdomain.html.in  | 19 ++
>>  docs/schemas/domaincommon.rng  | 10 
>>  src/bhyve/bhyve_driver.c   | 10 
>>  src/bhyve/bhyve_parse_command.c| 70 
>> ++
>>  src/bhyve/bhyve_process.c  | 22 +++
>>  src/conf/domain_conf.c | 41 +
>>  src/conf/domain_conf.h | 11 
>>  .../bhyveargv2xml-loader-stdin-file.args   |  9 +++
>>  .../bhyveargv2xml-loader-stdin-file.xml| 19 ++
>>  .../bhyveargv2xml-loader-stdin-multiline.args  | 13 
>>  .../bhyveargv2xml-loader-stdin-multiline.xml   | 21 +++
>>  .../bhyveargv2xml-loader-stdin-oneline.args| 11 
>>  .../bhyveargv2xml-loader-stdin-oneline.xml | 19 ++
>>  tests/bhyveargv2xmltest.c  |  3 +
>>  .../bhyvexml2argv-grub-stdin-file.args |  9 +++
>>  .../bhyvexml2argv-grub-stdin-file.devmap   |  1 +
>>  .../bhyvexml2argv-grub-stdin-file.ldargs   |  4 ++
>>  .../bhyvexml2argv-grub-stdin-file.xml  | 25 
>>  .../bhyvexml2argv-grub-stdin-multiline.args|  9 +++
>>  .../bhyvexml2argv-grub-stdin-multiline.devmap  |  1 +
>>  .../bhyvexml2argv-grub-stdin-multiline.ldargs  |  4 ++
>>  .../bhyvexml2argv-grub-stdin-multiline.xml | 30 ++
>>  .../bhyvexml2argv-grub-stdin-oneline.args  |  9 +++
>>  .../bhyvexml2argv-grub-stdin-oneline.devmap|  1 +
>>  .../bhyvexml2argv-grub-stdin-oneline.ldargs|  4 ++
>>  .../bhyvexml2argv-grub-stdin-oneline.xml   | 25 
>>  tests/bhyvexml2argvtest.c  |  3 +
>>  .../bhyvexml2xmlout-grub-stdin-file.xml| 34 +++
>>  .../bhyvexml2xmlout-grub-stdin-multiline.xml   | 39 
>>  .../bhyvexml2xmlout-grub-stdin-oneline.xml | 34 +++
>>  tests/bhyvexml2xmltest.c   |  3 +
>>  31 files changed, 513 insertions(+)
>>  create mode 100644 
>> tests/bhyveargv2xmldata/bhyveargv2xml-loader-stdin-file.args
>>  create mode 100644 
>> tests/bhyveargv2xmldata/bhyveargv2xml-loader-stdin-file.xml
>>  create mode 100644 
>> tests/bhyveargv2xmldata/bhyveargv2xml-loader-stdin-multiline.args
>>  create mode 100644 
>> tests/bhyveargv2xmldata/bhyveargv2xml-loader-stdin-multiline.xml
>>  create mode 100644 
>> tests/bhyveargv2xmldata/bhyveargv2xml-loader-stdin-oneline.args
>>  create mode 100644 
>> tests/bhyveargv2xmldata/bhyveargv2xml-loader-stdin-oneline.xml
>>  create mode 100644 
>> tests/bhyvexml2argvdata/bhyvexml2argv-grub-stdin-file.args
>>  create mode 100644 
>> tests/bhyvexml2argvdata/bhyvexml2argv-grub-stdin-file.devmap
>>  create mode 100644 
>> tests/bhyvexml2argvdata/bhyvexml2argv-grub-stdin-file.ldargs
>>  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-grub-stdin-file.xml
>>  create mode 100644 
>> tests/bhyvexml2argvdata/bhyvexml2argv-grub-stdin-multiline.args
>>  create mode 100644 
>> tests/bhyvexml2argvdata/bhyvexml2argv-grub-stdin-multiline.devmap
>>  create mode 100644 
>> tests/bhyvexml2argvdata/bhyvexml2argv-grub-stdin-multiline.ldargs
>>  create mode 100644

Re: [libvirt] [PATCH] bhyve: add support for passing stdin to loader

2018-04-26 Thread John Ferlan 


On 04/13/2018 03:27 PM, Fabian Freyer wrote:
> This commit adds the  node to the domain definition,
> with the following semantics:
> 
> To pass standard input verbatim to the bootloader, set
> 
> some stdin
> 
> Multiline standard input can be set using a CDATA tag:
> 
> 
> 
> Standard input can be read from a file as follows:
> 
> 

Not my area of expertise, but some feedback to hopefully help a bit
seeing as there have been no other takes for 2 weeks.

Personally this format over the other format would seem to be better
although the attribute name would be "path" not "file".  IOW: That whole
CDATA thing - not sure it passes by all the censors^W reviewers
scrutiny.  Is there a particular reason to have this CDATA tag syntax?
Parsing the line and making sure consumers provide a specific format is
probably more hassle than it's worth and I would think presents an
opportunity to insert things that may cause interesting errors. Any time
you accept something from stdin like that you open up buffer overflows
and buffer handling problems. Not that the same thing cannot be in the
file, but at least you're then passing that off to something else to
manage whether what's in a file is correctly formatted as you're not
validating the contents of the file, just that it exists.

> 
> Signed-off-by: Fabian Freyer 
> ---
>  docs/formatdomain.html.in  | 19 ++
>  docs/schemas/domaincommon.rng  | 10 
>  src/bhyve/bhyve_driver.c   | 10 
>  src/bhyve/bhyve_parse_command.c| 70 
> ++
>  src/bhyve/bhyve_process.c  | 22 +++
>  src/conf/domain_conf.c | 41 +
>  src/conf/domain_conf.h | 11 
>  .../bhyveargv2xml-loader-stdin-file.args   |  9 +++
>  .../bhyveargv2xml-loader-stdin-file.xml| 19 ++
>  .../bhyveargv2xml-loader-stdin-multiline.args  | 13 
>  .../bhyveargv2xml-loader-stdin-multiline.xml   | 21 +++
>  .../bhyveargv2xml-loader-stdin-oneline.args| 11 
>  .../bhyveargv2xml-loader-stdin-oneline.xml | 19 ++
>  tests/bhyveargv2xmltest.c  |  3 +
>  .../bhyvexml2argv-grub-stdin-file.args |  9 +++
>  .../bhyvexml2argv-grub-stdin-file.devmap   |  1 +
>  .../bhyvexml2argv-grub-stdin-file.ldargs   |  4 ++
>  .../bhyvexml2argv-grub-stdin-file.xml  | 25 
>  .../bhyvexml2argv-grub-stdin-multiline.args|  9 +++
>  .../bhyvexml2argv-grub-stdin-multiline.devmap  |  1 +
>  .../bhyvexml2argv-grub-stdin-multiline.ldargs  |  4 ++
>  .../bhyvexml2argv-grub-stdin-multiline.xml | 30 ++
>  .../bhyvexml2argv-grub-stdin-oneline.args  |  9 +++
>  .../bhyvexml2argv-grub-stdin-oneline.devmap|  1 +
>  .../bhyvexml2argv-grub-stdin-oneline.ldargs|  4 ++
>  .../bhyvexml2argv-grub-stdin-oneline.xml   | 25 
>  tests/bhyvexml2argvtest.c  |  3 +
>  .../bhyvexml2xmlout-grub-stdin-file.xml| 34 +++
>  .../bhyvexml2xmlout-grub-stdin-multiline.xml   | 39 
>  .../bhyvexml2xmlout-grub-stdin-oneline.xml | 34 +++
>  tests/bhyvexml2xmltest.c   |  3 +
>  31 files changed, 513 insertions(+)
>  create mode 100644 
> tests/bhyveargv2xmldata/bhyveargv2xml-loader-stdin-file.args
>  create mode 100644 
> tests/bhyveargv2xmldata/bhyveargv2xml-loader-stdin-file.xml
>  create mode 100644 
> tests/bhyveargv2xmldata/bhyveargv2xml-loader-stdin-multiline.args
>  create mode 100644 
> tests/bhyveargv2xmldata/bhyveargv2xml-loader-stdin-multiline.xml
>  create mode 100644 
> tests/bhyveargv2xmldata/bhyveargv2xml-loader-stdin-oneline.args
>  create mode 100644 
> tests/bhyveargv2xmldata/bhyveargv2xml-loader-stdin-oneline.xml
>  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-grub-stdin-file.args
>  create mode 100644 
> tests/bhyvexml2argvdata/bhyvexml2argv-grub-stdin-file.devmap
>  create mode 100644 
> tests/bhyvexml2argvdata/bhyvexml2argv-grub-stdin-file.ldargs
>  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-grub-stdin-file.xml
>  create mode 100644 
> tests/bhyvexml2argvdata/bhyvexml2argv-grub-stdin-multiline.args
>  create mode 100644 
> tests/bhyvexml2argvdata/bhyvexml2argv-grub-stdin-multiline.devmap
>  create mode 100644 
> tests/bhyvexml2argvdata/bhyvexml2argv-grub-stdin-multiline.ldargs
>  create mode 100644 
> tests/bhyvexml2argvdata/bhyvexml2argv-grub-stdin-multiline.xml
>  create mode 100644 
> tests/bhyvexml2argvdata/bhyvexml2argv-grub-stdin-oneline.args
>  create mode 100644 
> tests/bhyvexml2argvdata/bhyvexml2argv-grub-stdin-oneline.devmap
>  create mode 100644 
> tests/bhyvexml2argvdata/bhyvexml2argv-grub-stdin-oneline.ldargs
>  create mode 100644 
>