On Wed, Apr 25, 2018 at 09:50:19AM +0200, Kevin Wolf wrote:
> Am 24.04.2018 um 20:26 hat Jeff Cody geschrieben:
> > On Fri, Apr 20, 2018 at 04:39:02PM +0200, Markus Armbruster wrote:
> > > >> Ways to avoid the troublesome auth-cephx: {}:
> > > >>
> > > >> (A) Make auth-cephx a bool, rely on the other ways to provide the key
> > > >>
> > > >> (B) Make auth-cephx a bool and add the @key-secret member next to it
> > > >>
> > > >> Like @user, the member is meaningless with auth-none.
> > > >>
> > > >> (C) Make auth-cephx.key-secret a mandatory StrOrNull
> > > >>
> > > >> Should take care of "vanishes on flattening" problem, but dotted
> > > >> key
> > > >> syntax is still screwed: since any value is a valid string, there's
> > > >> none left for null. My take would be that if you want to say
> > > >> auth-cephx.key-secret: {}, you get to say it in JSON.
> > > >>
> > > >> Aside: check_alternate() tries to catch such alternates, but we
> > > >> failed to update it when we added first class null. *sigh*
> > > >>
> > > >> Which one do you hate least?
> > > >
> > > > What should happen with null when you stringify it? If we want to take
> > > > the options QDict, stringify all entries and run them through the keyval
> > > > visitor, I'm almost sure that null will be lost.
> > >
> > > For the stringify idea to work, the keyval visitor needs to map the
> > > string right back to the original value.
> > >
> > > The keyval visitor currently requires the value to be null, not a
> > > string.
> > >
> > > Therefore, the stringify operation must leave null alone. Not pretty,
> > > but works.
>
> Okay, I didn't know that the keyval visitor has any way to specify null.
> It doesn't really matter what the exact representation is as long as we
> can generate it. So sure, that's workable then.
>
> > > You might ask why not change the keyval visitor instead so it expects ""
> > > rather than null. That's no good, because it makes StrOrNull ambiguous.
> > >
> > > keyval.c can only create string scalars. I think "use JSON if you want
> > > to specify null" is still good enough. We can make keyval.c more
> > > expressive if we need to, but (1) I don't think we should block this
> > > work on it, and (2) see "hairy" above.
> > >
> > > > So (A) doesn't work unless we can specify what "other ways" are that are
> > > > acceptable for libvirt,
> > >
> > > Yes.
> > >
> > > > and (C) probably doesn't play well with b. above
> > > > (the qdict_stringify_entries() for handling mixed type QDicts).
> > >
> > > I think it could be done, and tried to sketch how.
> > >
> > > > Looks like only (B) is left as viable, so that's automatically the one I
> > > > hate least of these. If we can fix the problems, I think I'd prefer (C),
> > > > though.
> > >
> > > I could accept (B), in particular since it mirrors existing @user.
>
> I don't like (B) much because it adds additional rules which options
> must, may or can be present depending on the presence or value of other
> options. This kind of dependencies should be visible in the schema with
> nested structs and unions, and checked for consistency by QAPI, rather
> than being checked individually in .bdrv_open() implementations.
>
> As for @user, you had sources to confirm that it is indeed irrelevant
> for 'none', so I'd rather do the opposite thing and move it to
> RbdAuthCephx.
>
> > > I'm happy to help with exploring (C).
> > >
> > > What's the next step?
> >
> > My preference is (B). Primarily because I don't like the idea of breaking
> > dotted key syntax for null keys. I'd rather see something more verbose like
> > (B), that can still be navigated correctly both ways.
>
> Yes, it's not perfect, but it doesn't make any functionality unavailable
> because you can always using JSON, even on the command line. Dotted
> syntax is nicer for manual use, but in this specific case I think null
> will be the default, so there is no need to specify it explicitly
> anyway - neither with dotted key syntax nor with JSON.
>
Good point on the default case, that essentially negates my concerns.
> I prefer slightly unwieldy command line syntax to getting the internally
> used data types wrong (= hard to use correctly).
>
Fair enough.
> > How about I put together a patch with (B) for an RFC v2?
>
> How about doing the same with (C) and moving @user? :-)
>
Sounds like a plan!
-Jeff
