Libguestfs - Jan 2020 - Re: [common PATCH] Trim whitespaces from commands read from file

On Thu, Jan 23, 2020 at 02:04:53PM +0100, Martin Kletzander
wrote:
> On Thu, Jan 23, 2020 at 12:16:50PM +0100, Martin Kletzander wrote:
> >Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> 
> I forgot to mention this is trying to fix the following BZ:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1351000
> >---
> >mlcustomize/customize_cmdline.ml | 1 +
> >1 file changed, 1 insertion(+)
> >
> >diff --git a/mlcustomize/customize_cmdline.ml
b/mlcustomize/customize_cmdline.ml
> >index c062379879e2..67e85af2ad93 100644
> >--- a/mlcustomize/customize_cmdline.ml
> >+++ b/mlcustomize/customize_cmdline.ml
> >@@ -481,6 +481,7 @@ let rec argspec () > >    ] in
> >    let lines = read_whole_file filename in
> >    let lines = String.lines_split lines in
> >+    let lines = List.map String.trim lines in
I wonder if String.triml is safer?  However I cannot
think of a way that it's likely to be useful to have
trailing whitespace be meaningful.

Please put this in the subject line of the commit
message: “(RHBZ#1351000)”.  This will ensure that it
is picked up by the release notes by the
bugs-in-changelog script.

ACK

Rich.
> >    let lines = List.filter (
> >      fun line ->
> >        String.length line > 0 && line.[0] <>
'#'
> >-- 
> >2.25.0
> >
> >_______________________________________________
> >Libguestfs mailing list
> >Libguestfs@redhat.com
> >https://www.redhat.com/mailman/listinfo/libguestfs


-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On Thu, Jan 23, 2020 at 03:16:47PM +0000, Richard W.M. Jones
wrote:
>On Thu, Jan 23, 2020 at 02:04:53PM +0100, Martin Kletzander wrote:
>> On Thu, Jan 23, 2020 at 12:16:50PM +0100, Martin Kletzander wrote:
>> >Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>>
>> I forgot to mention this is trying to fix the following BZ:
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1351000
>
>> >---
>> >mlcustomize/customize_cmdline.ml | 1 +
>> >1 file changed, 1 insertion(+)
>> >
>> >diff --git a/mlcustomize/customize_cmdline.ml
b/mlcustomize/customize_cmdline.ml
>> >index c062379879e2..67e85af2ad93 100644
>> >--- a/mlcustomize/customize_cmdline.ml
>> >+++ b/mlcustomize/customize_cmdline.ml
>> >@@ -481,6 +481,7 @@ let rec argspec () >> >    ] in
>> >    let lines = read_whole_file filename in
>> >    let lines = String.lines_split lines in
>> >+    let lines = List.map String.trim lines in
>
>I wonder if String.triml is safer?  However I cannot
>think of a way that it's likely to be useful to have
>trailing whitespace be meaningful.
>
I actually thought about that and it might be dangerous to just trim from left.
Space in the end might change semantics of a command (although I did not go
through the commands), consider something like "cp a b " which would
end up
being split into ["cp"; "a"; "b"; ""]. 
In this particular case it would
probably fail, but you know what I mean, right?
>Please put this in the subject line of the commit
>message: “(RHBZ#1351000)”.  This will ensure that it
>is picked up by the release notes by the
>bugs-in-changelog script.
>
OK, probably the same thing needs to be added in the commit that updates the
submodule in libguestfs repo, right?
>ACK
>
>Rich.
>
>> >    let lines = List.filter (
>> >      fun line ->
>> >        String.length line > 0 && line.[0] <>
'#'
>> >--
>> >2.25.0
>> >
>> >_______________________________________________
>> >Libguestfs mailing list
>> >Libguestfs@redhat.com
>> >https://www.redhat.com/mailman/listinfo/libguestfs
>
>
>
>-- 
>Richard Jones, Virtualization Group, Red Hat
http://people.redhat.com/~rjones
>Read my programming and virtualization blog: http://rwmj.wordpress.com
>virt-top is 'top' for virtual machines.  Tiny program with many
>powerful monitoring features, net stats, disk stats, logging, etc.
>http://people.redhat.com/~rjones/virt-top

On Thu, Jan 23, 2020 at 10:32:17PM +0100, Martin Kletzander
wrote:
>On Thu, Jan 23, 2020 at 03:16:47PM +0000, Richard W.M. Jones wrote:
>>On Thu, Jan 23, 2020 at 02:04:53PM +0100, Martin Kletzander wrote:
>>> On Thu, Jan 23, 2020 at 12:16:50PM +0100, Martin Kletzander wrote:
>>> >Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>>>
>>> I forgot to mention this is trying to fix the following BZ:
>>>
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1351000
>>
>>> >---
>>> >mlcustomize/customize_cmdline.ml | 1 +
>>> >1 file changed, 1 insertion(+)
>>> >
>>> >diff --git a/mlcustomize/customize_cmdline.ml
b/mlcustomize/customize_cmdline.ml
>>> >index c062379879e2..67e85af2ad93 100644
>>> >--- a/mlcustomize/customize_cmdline.ml
>>> >+++ b/mlcustomize/customize_cmdline.ml
>>> >@@ -481,6 +481,7 @@ let rec argspec () >>> >    ]
in
>>> >    let lines = read_whole_file filename in
>>> >    let lines = String.lines_split lines in
>>> >+    let lines = List.map String.trim lines in
>>
>>I wonder if String.triml is safer?  However I cannot
>>think of a way that it's likely to be useful to have
>>trailing whitespace be meaningful.
>>
>
>I actually thought about that and it might be dangerous to just trim from
left.
>Space in the end might change semantics of a command (although I did not go
>through the commands), consider something like "cp a b " which
would end up
>being split into ["cp"; "a"; "b";
""].  In this particular case it would
>probably fail, but you know what I mean, right?
>
>>Please put this in the subject line of the commit
>>message: “(RHBZ#1351000)”.  This will ensure that it
>>is picked up by the release notes by the
>>bugs-in-changelog script.
>>
>
I changed the subject to:

mlcustomize: Trim whitespaces from commands read from file (RHBZ#1351000)

and then realized I do not have commit access.  Could anyone fix-up the subject
and push it?  Thanks
>OK, probably the same thing needs to be added in the commit that updates the
>submodule in libguestfs repo, right?
>
Should I send a patch so that we don't forget or is it going to be picked up
anyway?
>>ACK
>>
>>Rich.
>>
>>> >    let lines = List.filter (
>>> >      fun line ->
>>> >        String.length line > 0 && line.[0] <>
'#'
>>> >--
>>> >2.25.0
>>> >
>>> >_______________________________________________
>>> >Libguestfs mailing list
>>> >Libguestfs@redhat.com
>>> >https://www.redhat.com/mailman/listinfo/libguestfs
>>
>>
>>
>>--
>>Richard Jones, Virtualization Group, Red Hat
http://people.redhat.com/~rjones
>>Read my programming and virtualization blog: http://rwmj.wordpress.com
>>virt-top is 'top' for virtual machines.  Tiny program with many
>>powerful monitoring features, net stats, disk stats, logging, etc.
>>http://people.redhat.com/~rjones/virt-top

>_______________________________________________
>Libguestfs mailing list
>Libguestfs@redhat.com
>https://www.redhat.com/mailman/listinfo/libguestfs

On Thu, Jan 23, 2020 at 10:32:17PM +0100, Martin Kletzander
wrote:
> On Thu, Jan 23, 2020 at 03:16:47PM +0000, Richard W.M. Jones wrote:
> >Please put this in the subject line of the commit
> >message: “(RHBZ#1351000)”.  This will ensure that it
> >is picked up by the release notes by the
> >bugs-in-changelog script.
> >
> 
> OK, probably the same thing needs to be added in the commit that updates
the
> submodule in libguestfs repo, right?
The script is fairly braindead, so as long as one (RHBZ#..)
string appears it will work:

https://github.com/libguestfs/libguestfs/blob/master/bugs-in-changelog.sh

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On Thu, Jan 23, 2020 at 03:16:47PM +0000, Richard W.M. Jones
wrote:
>On Thu, Jan 23, 2020 at 02:04:53PM +0100, Martin Kletzander wrote:
>> On Thu, Jan 23, 2020 at 12:16:50PM +0100, Martin Kletzander wrote:
>> >Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>>
>> I forgot to mention this is trying to fix the following BZ:
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1351000
>
>> >---
>> >mlcustomize/customize_cmdline.ml | 1 +
>> >1 file changed, 1 insertion(+)
>> >
>> >diff --git a/mlcustomize/customize_cmdline.ml
b/mlcustomize/customize_cmdline.ml
>> >index c062379879e2..67e85af2ad93 100644
>> >--- a/mlcustomize/customize_cmdline.ml
>> >+++ b/mlcustomize/customize_cmdline.ml
>> >@@ -481,6 +481,7 @@ let rec argspec () >> >    ] in
>> >    let lines = read_whole_file filename in
>> >    let lines = String.lines_split lines in
>> >+    let lines = List.map String.trim lines in
>
>I wonder if String.triml is safer?  However I cannot
>think of a way that it's likely to be useful to have
>trailing whitespace be meaningful.
>
>Please put this in the subject line of the commit
>message: “(RHBZ#1351000)”.  This will ensure that it
>is picked up by the release notes by the
>bugs-in-changelog script.
>
>ACK
>
I found that this is still not enough to make it behave in a nice way.  What we
probably want is the split, which happens few lines lower, to actually behave
similar to Str.split, which actually splits by any number of the delimiters (so
that "asdf fdsa" is split into ["asdf"; "fdsa"]
and not ["asdf"; ""; "fdsa"].

I know this fix is not important at all, but since I wanted to exercise some
OCaml, I will try to fix that implementation instead.

Do any one of you know of a part of any libguestfs tool which would depend on
the current, too strict, behaviour?
>Rich.
>
>> >    let lines = List.filter (
>> >      fun line ->
>> >        String.length line > 0 && line.[0] <>
'#'
>> >--
>> >2.25.0
>> >
>> >_______________________________________________
>> >Libguestfs mailing list
>> >Libguestfs@redhat.com
>> >https://www.redhat.com/mailman/listinfo/libguestfs
>
>
>
>-- 
>Richard Jones, Virtualization Group, Red Hat
http://people.redhat.com/~rjones
>Read my programming and virtualization blog: http://rwmj.wordpress.com
>virt-top is 'top' for virtual machines.  Tiny program with many
>powerful monitoring features, net stats, disk stats, logging, etc.
>http://people.redhat.com/~rjones/virt-top

On Wed, Jan 29, 2020 at 01:12:51PM +0100, Martin Kletzander
wrote:
>On Thu, Jan 23, 2020 at 03:16:47PM +0000, Richard W.M. Jones wrote:
>>On Thu, Jan 23, 2020 at 02:04:53PM +0100, Martin Kletzander wrote:
>>> On Thu, Jan 23, 2020 at 12:16:50PM +0100, Martin Kletzander wrote:
>>> >Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>>>
>>> I forgot to mention this is trying to fix the following BZ:
>>>
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1351000
>>
>>> >---
>>> >mlcustomize/customize_cmdline.ml | 1 +
>>> >1 file changed, 1 insertion(+)
>>> >
>>> >diff --git a/mlcustomize/customize_cmdline.ml
b/mlcustomize/customize_cmdline.ml
>>> >index c062379879e2..67e85af2ad93 100644
>>> >--- a/mlcustomize/customize_cmdline.ml
>>> >+++ b/mlcustomize/customize_cmdline.ml
>>> >@@ -481,6 +481,7 @@ let rec argspec () >>> >    ]
in
>>> >    let lines = read_whole_file filename in
>>> >    let lines = String.lines_split lines in
>>> >+    let lines = List.map String.trim lines in
>>
>>I wonder if String.triml is safer?  However I cannot
>>think of a way that it's likely to be useful to have
>>trailing whitespace be meaningful.
>>
>>Please put this in the subject line of the commit
>>message: “(RHBZ#1351000)”.  This will ensure that it
>>is picked up by the release notes by the
>>bugs-in-changelog script.
>>
>>ACK
>>
>
>I found that this is still not enough to make it behave in a nice way.  What
we
>probably want is the split, which happens few lines lower, to actually
behave
>similar to Str.split, which actually splits by any number of the delimiters
(so
>that "asdf fdsa" is split into ["asdf";
"fdsa"] and not ["asdf"; ""; "fdsa"].
>
OK, my bad, this part of the code only splits the command from the arguments.
That way it also makes more sense to use ltrim.  Will send a final patch so that
I get myself out of this mess =)
>I know this fix is not important at all, but since I wanted to exercise some
>OCaml, I will try to fix that implementation instead.
>
>Do any one of you know of a part of any libguestfs tool which would depend
on
>the current, too strict, behaviour?
>
>>Rich.
>>
>>> >    let lines = List.filter (
>>> >      fun line ->
>>> >        String.length line > 0 && line.[0] <>
'#'
>>> >--
>>> >2.25.0
>>> >
>>> >_______________________________________________
>>> >Libguestfs mailing list
>>> >Libguestfs@redhat.com
>>> >https://www.redhat.com/mailman/listinfo/libguestfs
>>
>>
>>
>>--
>>Richard Jones, Virtualization Group, Red Hat
http://people.redhat.com/~rjones
>>Read my programming and virtualization blog: http://rwmj.wordpress.com
>>virt-top is 'top' for virtual machines.  Tiny program with many
>>powerful monitoring features, net stats, disk stats, logging, etc.
>>http://people.redhat.com/~rjones/virt-top

On Wed, Jan 29, 2020 at 01:12:51PM +0100, Martin Kletzander
wrote:
> On Thu, Jan 23, 2020 at 03:16:47PM +0000, Richard W.M. Jones wrote:
> >On Thu, Jan 23, 2020 at 02:04:53PM +0100, Martin Kletzander wrote:
> >>On Thu, Jan 23, 2020 at 12:16:50PM +0100, Martin Kletzander wrote:
> >>>Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> >>
> >>I forgot to mention this is trying to fix the following BZ:
> >>
> >>https://bugzilla.redhat.com/show_bug.cgi?id=1351000
> >
> >>>---
> >>>mlcustomize/customize_cmdline.ml | 1 +
> >>>1 file changed, 1 insertion(+)
> >>>
> >>>diff --git a/mlcustomize/customize_cmdline.ml
b/mlcustomize/customize_cmdline.ml
> >>>index c062379879e2..67e85af2ad93 100644
> >>>--- a/mlcustomize/customize_cmdline.ml
> >>>+++ b/mlcustomize/customize_cmdline.ml
> >>>@@ -481,6 +481,7 @@ let rec argspec () > >>>    ]
in
> >>>    let lines = read_whole_file filename in
> >>>    let lines = String.lines_split lines in
> >>>+    let lines = List.map String.trim lines in
> >
> >I wonder if String.triml is safer?  However I cannot
> >think of a way that it's likely to be useful to have
> >trailing whitespace be meaningful.
> >
> >Please put this in the subject line of the commit
> >message: “(RHBZ#1351000)”.  This will ensure that it
> >is picked up by the release notes by the
> >bugs-in-changelog script.
> >
> >ACK
> >
> 
> I found that this is still not enough to make it behave in a nice way. 
What we
> probably want is the split, which happens few lines lower, to actually
behave
> similar to Str.split, which actually splits by any number of the delimiters
(so
> that "asdf fdsa" is split into ["asdf";
"fdsa"] and not ["asdf"; ""; "fdsa"].
> 
> I know this fix is not important at all, but since I wanted to exercise
some
> OCaml, I will try to fix that implementation instead.
> 
> Do any one of you know of a part of any libguestfs tool which would depend
on
> the current, too strict, behaviour?
Almost everything.  You can't change that library function, add a new
one instead.

Rich.
> >Rich.
> >
> >>>    let lines = List.filter (
> >>>      fun line ->
> >>>        String.length line > 0 && line.[0] <>
'#'
> >>>--
> >>>2.25.0
> >>>
> >>>_______________________________________________
> >>>Libguestfs mailing list
> >>>Libguestfs@redhat.com
> >>>https://www.redhat.com/mailman/listinfo/libguestfs
> >
> >
> >
> >-- 
> >Richard Jones, Virtualization Group, Red Hat
http://people.redhat.com/~rjones
> >Read my programming and virtualization blog: http://rwmj.wordpress.com
> >virt-top is 'top' for virtual machines.  Tiny program with many
> >powerful monitoring features, net stats, disk stats, logging, etc.
> >http://people.redhat.com/~rjones/virt-top


-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/