Re: [Xen-devel] [libvirt] Libvirt config converter can't handle file not ending with new line

[Jim Fehlig <jfehlig@xxxxxxxx>]

On 11/07/2017 08:54 AM, Wim ten Have wrote:
> On Tue, 7 Nov 2017 12:20:05 +0000
> Wei Liu <wei.liu2@xxxxxxxxxx> wrote:
>
> > On Mon, Nov 06, 2017 at 09:41:01PM -0700, Jim Fehlig wrote:
> > > On 10/30/2017 06:17 AM, Wei Liu wrote:
> > > > Hi Jim
> > > >
> > > > I discover a problem when using xen_xl converter. When the file in
> > > > question doesn't end with a new line, I get the following error:
> > > >
> > > >     error: configuration file syntax error: memory conf:53: expecting a value
> > > I'm not able to reproduce this issue. The libvirt.git tree I tried was a bit
> > > dated, but even after updating to latest master I can't reproduce.
> > >    
> > > > After digging a bit (but haven't read libvirt code), it appears that the
> > > > file didn't end with a new line.
> > > I tried several files without ending new lines, going both directions
> > > (domxml-to-native and domxml-from-native), but didn't see the mentioned
> > > error. Perhaps your config is revealing another bug which is being
> > > improperly reported. Can you provide an example of the problematic config?
> > >    
> > I tried to get the exact file that caused the problem but it is already
> > destroyed by osstest.
> >
> > A similar file:
> >
> > http://logs.test-lab.xenproject.org/osstest/logs/115436/test-amd64-amd64-libvirt-pair/debian.guest.osstest.cfg
> >
> > If you hexdump -C it, you can see the last character is 0a. Remove it and
> > feed the file into the converter.
> > Wei.
>    The phenonomem you point out is indeed weird.  And my first response
>    is that this is a bug parsing the cfg input.  I did little explore and
>    think that src/util/virconf.c (virConfParseLong(), virConfParseValue())
>    should be reworked as pointed out in below context diffs.
>
>         <wtenhave@nina:140> git diff
>         diff --git a/src/util/virconf.c b/src/util/virconf.c
>         index 39c2bd917..bc8e57ec3 100644
>         --- a/src/util/virconf.c
>         +++ b/src/util/virconf.c
>         @@ -352,7 +352,7 @@ virConfParseLong(virConfParserCtxtPtr ctxt, long 
> long *val)
>              } else if (CUR == '+') {
>                  NEXT;
>              }
>         -    if ((ctxt->cur >= ctxt->end) || (!c_isdigit(CUR))) {
>         +    if ((ctxt->cur > ctxt->end) || (!c_isdigit(CUR))) {
>                  virConfError(ctxt, VIR_ERR_CONF_SYNTAX, _("unterminated 
> number"));
>                  return -1;
>              }
>         @@ -456,7 +456,7 @@ virConfParseValue(virConfParserCtxtPtr ctxt)
>              long long l = 0;
>         
>              SKIP_BLANKS;
>         -    if (ctxt->cur >= ctxt->end) {
>         +    if (ctxt->cur > ctxt->end) {
>                  virConfError(ctxt, VIR_ERR_CONF_SYNTAX, _("expecting a 
> value"));
>                  return NULL;
>              }
>
>    I did not go beyond this yet.
Thanks Wim. I noticed Cole fixed a similar issue when parsing content from a 
file with commit 3cc2a9e0d4. But I think instead of replicating that fix in 
virConfReadString(), we should just set the end of content correctly in 
virConfParse(). I've sent a patch along those lines that fixes Wei's test case 
and doesn't regress Cole's test case
https://www.redhat.com/archives/libvir-list/2017-November/msg00286.html

Regards,
Jim


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel