nvpairs without value are not allowed about crmsh HOT 13 CLOSED

On Fri, Nov 28, 2014 at 05:23:50AM -0800, Kristoffer Grönlund wrote:

The crmsh syntax no longer allows only foo instead of foo="" as nvpair value assignments.

I don't want to allow just foo as this limits the possible
extensions of the language too much,

Why do you think so, given that such no-value attributes can
appear only in the nvpair sets?

but enabling the use of a
trailing = to signify empty values should be allowed.
That is, foo= rather than foo.

How would you tell foo= from foo=""? Even though it seems
possible to distinguish the two, given that the crmsh tries not
to output quotes around values, the difference seems to be
diminishingly small ;-)

Not that I'm very happy with such no-value attributes, but if
they were once allowed, I think that we should continue to
support them as they were. Otherwise, exports from crmsh v1.x
won't work any more with the new one.

I'm not sure how much is usage of such attributes widespread
though. In case it's really too much trouble to support.

from crmsh.

There are several ambiguities that in practice may not be an issue but that I dislike as a parser writer ;) For example, any resource parameter named "params", "meta", "operations", "op" or "rule" cannot be distinguished from what the CIB syntax parses as the beginning of a new parameter/meta/etc section.

You are right, foo= and foo="" are the same.

from crmsh.

Haha yes, actually, my modification was a no-op. This syntax was already accepted... doh! Anyway, that's good. At least there is another test case for this now.

from crmsh.

Personally I am somewhat okay with old exports not being accepted anymore, as long as the error message is clear about what is no longer accepted. The old parser accepted a lot of invalid syntax which is now no longer accepted as well. As long as someone who is importing old CIB syntax gets an understandable error message and can fix the problem, it should be fine I hope. It is worse if the new version silently misinterprets old exports, like if it would silently ignore a line it doesn't understand... But maybe you disagree. :)

from crmsh.

On Fri, Nov 28, 2014 at 05:46:28AM -0800, Kristoffer Grönlund wrote:

There are several ambiguities that in practice may not be an issue but that I dislike as a parser writer ;)

Indeed, as I already said, I was also quite a bit perplexed with
this case. But what is to be done if one wants to support such
cases?

For example, any resource parameter named "params", "meta", "operations", "op" or "rule" cannot be distinguished from what the CIB syntax parses as the beginning of a new parameter/meta/etc section.

Whoever names parameters the same as crmsh reserved keywords
deserves what he gets :) Seriously though, that would be as
confusing as it gets, with or without parameter mismatches.

Perhaps we need new syntax. This one is definitely not so hot.
What do you say? Something yaml-like perhaps. Or similar to
named.conf.

from crmsh.

On Fri, Nov 28, 2014 at 05:51:46AM -0800, Kristoffer Grönlund wrote:

Personally I am somewhat okay with old exports not being accepted anymore, as long as the error message is clear about what is no longer accepted. The old parser accepted a lot of invalid syntax which is now no longer accepted as well. As long as someone who is importing old CIB syntax gets an understandable error message and can fix the problem, it should be fine I hope. It is worse if the new version silently misinterprets old exports, like if it would silently ignore a line it doesn't understand...

Yes, but such an option is not really to be considered.

But maybe you disagree. :)

I do. Syntax improvements are fine, but we do need to support
previous version format for imports. Not doing so can be very
irritating, good error reporting notwithstanding. There are
going to be quite a few upgrading to v2.x and they are going to
expect that their backups work.

It's just that this particular thing may be seldom used.

from crmsh.

On Fri, Nov 28, 2014 at 3:24 PM, Dejan Muhamedagic <[email protected]

wrote:

On Fri, Nov 28, 2014 at 05:46:28AM -0800, Kristoffer Grönlund wrote:

There are several ambiguities that in practice may not be an issue but
that I dislike as a parser writer ;)

Indeed, as I already said, I was also quite a bit perplexed with
this case. But what is to be done if one wants to support such
cases?

For example, any resource parameter named "params", "meta",
"operations", "op" or "rule" cannot be distinguished from what the CIB
syntax parses as the beginning of a new parameter/meta/etc section.

Whoever names parameters the same as crmsh reserved keywords
deserves what he gets :) Seriously though, that would be as
confusing as it gets, with or without parameter mismatches.

Well, that is true, but I am also of the opinion that an ambiguous syntax
is more difficult for humans to understand as well.

---

Perhaps we need new syntax. This one is definitely not so hot.
What do you say? Something yaml-like perhaps. Or similar to
named.conf.

Well, I think under the circumstances the CLI syntax is actually pretty
good :) There are just a few ambiguities like this. yaml is very difficult
to parse, I am not sure that it would really improve things. The real
problem is having to be able to translate both ways to and from XML, there
is no way to express something that doesn't map directly to XML. For
example the comments in the CIB are a bit of a hack and you can't rearrange
the lines as you want since the relative positioning doesn't have any
mapping in the XML. Those kind of things are what I would want to fix, but
those couldn't be fixed as long as we have this restriction.

Maybe a reimplemented CIB that uses something like yaml directly and
doesn't map to XML would be the solution, but that is not something I would
like to implement ;)

—
Reply to this email directly or view it on GitHub
#71 (comment).

from crmsh.

On Fri, Nov 28, 2014 at 3:30 PM, Dejan Muhamedagic <[email protected]

wrote:

On Fri, Nov 28, 2014 at 05:51:46AM -0800, Kristoffer Grönlund wrote:

Personally I am somewhat okay with old exports not being accepted
anymore, as long as the error message is clear about what is no longer
accepted. The old parser accepted a lot of invalid syntax which is now no
longer accepted as well. As long as someone who is importing old CIB syntax
gets an understandable error message and can fix the problem, it should be
fine I hope. It is worse if the new version silently misinterprets old
exports, like if it would silently ignore a line it doesn't understand...

Yes, but such an option is not really to be considered.

But maybe you disagree. :)

I do. Syntax improvements are fine, but we do need to support
previous version format for imports. Not doing so can be very
irritating, good error reporting notwithstanding. There are
going to be quite a few upgrading to v2.x and they are going to
expect that their backups work.

It's just that this particular thing may be seldom used.

This is the only case of previously valid syntax being rejected that I
know, so maybe I can live with making sure it also works. It should be
straight-forward to implement.

—
Reply to this email directly or view it on GitHub
#71 (comment).

from crmsh.

On Fri, Nov 28, 2014 at 06:55:21AM -0800, Kristoffer Grönlund wrote:
[...]

This is the only case of previously valid syntax being rejected that I
know,

Had I seen that happen earlier, I would've definitely
complained. ;-)

from crmsh.

Note: it is apparently valid XML to have no value attribute at all.
The xml.orig -> crmsh -> xml.re-converted sanity check bombs out,
because re-converted then contains a >> value="" << attribute that is not pressend in orig.

See below:

DEBUG: found pacemaker version: 1.1.12
DEBUG: nvpair!=nvpair: attributes differ: {'id': 'stonith_ipmi-karl-instance_attributes-verbose', 'name': 'verbose'} != {'name': 'verbose', 'value': '', 'id': 'stonith_ipmi-karl-instance_attributes-verbose'}

DEBUG: validation failed: <instance_attributes id="stonith_ipmi-karl-instance_attributes"></instance_attributes><meta_attributes id="stonith_ipmi-karl-meta_attributes"></meta_attributes>

-> primitive stonith_ipmi-karl stonith:fence_ipmilan params pcmk_host_list=karl verbose action=reboot ipaddr=10.43.242.221 login=root passwd=dummy method=onoff op start interval=0 timeout=60 op stop interval=0 timeout=60 op monitor interval=600 timeout=60 meta target-role=Started

-> <instance_attributes id="stonith_ipmi-karl-instance_attributes"><nvpair name="verbose" value=""

^^ THERE.

id="stonith_ipmi-karl-instance_attributes-verbose"/></instance_attributes><meta_attributes id="stonith_ipmi-karl-meta_attributes"></meta_attributes>

DEBUG: object stonith_ipmi-karl cannot be represented in the CLI notation

The thing is: the agent apparently wants to be called with "--verbose".
NOT --verbose="".

Any chance supporting that, too?

from crmsh.

Hmm, yes, if that is valid, I guess we should support it. But I am not happy about it! ;) The syntax will be even more ambiguous, but I guess it can't be avoided.

from crmsh.

@lge Could you test this version (#2a45993) with your CIB so I can be sure that this fixes it?

from crmsh.

This should be fixed now I hope. :)

from crmsh.