[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



[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 hollants com>


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]