[libvirt] [PATCH RESEND] Add support for <option> tag in network config

[Pieter Hollants]

[Bcc problems] Ah, indeed, I never got some of those mails, just saw my 
original patch merged last Sunday and already wondered why nobody 
commented :)

For brevity, I'll summarize the points we've been discussing. Forgive me 
if I oversee something important.

- We're thinking about importing DHCP options as "first class citizens" 
into our XML language.
   - Issue: There are no official names in any RFC.
     - Possible solution: use what dnsmasq offers ("dnsmasq --help dhcp").
       - Issue here: We do not want to depend on dnsmasq peculiariaties
         too much. Consider the situation where we choose a different
         DHCP server and that one uses different names. We'd have to
         convert from nameA to nameB.
       - Issue here: The selection made by dnsmasq could be called
         somewhat subjective. There is a great possibility that users
         need additional options, be it more rarely used,
         hardware-specific options for some access point, or the custom
         option range (224-252).
         - Possible solution: go with names for everything dnsmasq also
           has a name for and allow numbers for other DHCP options.
       - Issue here: If we take this route, we need to keep the names
         synchronous to dnsmasq. Eg. if dnsmasq learns a new option, we
         should add it to our XML schema as well for consistency reasons.
     - Possible solution: invent own names for further options.
       - Issue here: Not much utility and names won't be too intuitive.
     - Issue: whatever names we take, it hurts much more if in retrospect
       we chose the "wrong" ones than it does to allow <option>. So while
       allowing numbers at first sight looks like a possible trapdoor,
       I guess getting the names right is harder because it's sort of a
       decision "for eternity".
   - Issue: If we learn names, we might want to validate values just as
     well.
     - Issue: One could implement different depths of option validation.
     For example, according to RFC 4578 DHCP option 94 (Client network
     interface identifier) has a three octets-sized value and currently
     the first one can only be 1. Would we enforce this or just enforce
     three integer octets?
     - Issue: As a variation, some clients might not be 100%
     RFC-conformant and expect certain option values that we forbid
     because _we_ try to conform 100% to the appropriate RFC.
     - Issue: We would have to select for WHICH options we'd do
     validation. The underlying DHCP server, be it dnsmasq or something
     else in the future, is possibly not as restrictive as we are. The
     alternative approach "be as smart as dnsmasq is" will be hard to
     maintain.
     - Issue: Validation can't be done for the "private use" DHCP
     options.
     - Possible solution: Just allow anything for THESE.

Unrelated to this discussion there was the idea to add a "force=" 
attribute. I don't think there's much argument about it, it's a good idea.

Am 26.02.2013 19:05, schrieb Laine Stump:
> On the other hand, I don't want to over-engineer this problem so much
> that we never push *anything*.

Yes, unless I missed something I think the bottom line of my summary 
above is that there would have to be a really good argument _against_ 
"number=xxx". Implementing "name=yyy" does not automatically mean that 
"number=xxx" is a bad thing to do. Most of all, it's a very pragmatic 
solution whereas "name=xxx" most probably requires some design decisions.

> Without even arriving at a decision about this, I'm now thinking that
> maybe we should revert the earlier <option> patch until after the
> release, and re-push it after the named-option support is done
> (potentially with some changes).
>
> Any other opinions?

I'm used to having to compile it myself anyway, so that'd be fine with 
me if we want
to give it more thought.

On the other hand, I don't see that we can do WITHOUT "number=..." 
unless we want to implement only a restricted subset of DHCP options 
known by name.

>>> +                                      ipdef->options[r].number,
>>> +                                      ipdef->options[r].value);
>
> We've done no sanity checking on the contents of value. Is there any
> danger of a rogue metacharacter in dnsmasq.conf causing problems? I seem
> to recall asking that question before and being told "no", and I'm going
> to continue assuming that for now, but I still think it would be good
> practice to do some basic validation of the value if we can determine an
> all-encompassing valid character set that is something smaller than
> "everything" :-)

dnsmasq (or any other DHCP server we support in the future) could use a 
"test config" mode that simply returns success or failure and that we 
could use when reading in XML fragments. That way, we wouldn't have to 
duplicate dnsmasq's work.

>>> @@ -959,6 +968,7 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
>>>       virBufferAsprintf(&configbuf, "addn-hosts=%s\n",
>>>                         dctx->addnhostsfile->path);
>>>
>>> +
>> This looks odd.

And it got overseen during the merge ;)

>> Overall status, I'm tentative to give ACK. However, I'd like to hear one more opinion on the correctness of XML schema. Elder libvirt developers remember times when XML schema change needed ACK from at least one of Daniels :)
>
> Well, I'm usually hesitant about any xml addition for fear of getting it
> wrong, but since it was me who originally suggested this schema, and
> there was no negative comment about it on the list at the time, I'm
> giving my ACK (even though I'm not lucky enough to be named Daniel :-)).

I guess sometimes even after intensive discussions we just have to try 
and see if things work out.

-- 
Pieter Hollants <pieter at hollants.com>