Hi Laine,
thanks for your reply. Comments inline...
On 03/25/2011 10:02 PM, Laine Stump wrote:
> I haven't had time yet to look at the code in detail, but thought I
> should send this preliminary commentary.
>
> On 03/24/2011 09:58 AM, Michal Novotny wrote:
>> Hi,
>> this is the patch to add DNS TXT record support to libvirt networking
>> driver since this is feature that's supported by DNSMasq that's being
>> used by the bridge driver.
>>
>> Maybe you fail to understand the reasons why to implement such a feature
>> however it's a good thing IMHO since user could provide some information
>> in the DNS TXT record headers.
> As a matter of fact, I think that not only is this useful, but
> configuring other capabilities presented by dnsmasq would be good. I
> think you'll find a kindred spirit in PaweÅ KrzeÅniak, who was also
> wanting some other dnsmasq capabilities exposed (I forget which now).
Well, I have to agree that there are some options/capabilities that
would be good to be configurable.
Now, the dnsmasq command line is:
/usr/sbin/dnsmasq --strict-order --bind-interfaces
--pid-file=/var/run/libvirt/network/default.pid --conf-file=
--except-interface lo --listen-address 192.168.122.1 --dhcp-range
192.168.122.2,192.168.122.254
--dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases
--dhcp-lease-max=253 --dhcp-no-override
where only listen-address and just some DHCP related options are
configurable using the XML -> like dhcp-range, I don't know whether we
need to configure lease file and dhcp-lease-max value is being
configurable indirectly since if we have the 192.168.122.0/24 network
(defined by gateway IP address of 192.168.122.1, netmask of
255.255.255.0 and dhcp range from .2 to .254 because of reserved IP
addresses) we can easily calculate the DHCP lease max value.
The option that would be worth considering to be configurable could be
--dhcp-no-override option for this but I don't know whether this could
be useful. There's the description from DNSMasq man page:
--dhcp-no-override
Disable re-use of the DHCP servername and filename fields as extra
option space. If it can, dnsmasq moves the boot server and filename
information (from dhcp-boot) out of their dedicated fields into DHCP
options. This make extra space available in the DHCP packet for options
but can, rarely, confuse old or broken clients. This flag forces "simple
and safe" behaviour to avoid problems in such a case.
So I don't know whether we really need this configurable although it may
be a good thing.
For other DNSMasq option it may be good to make --local-ttl (and/or
--neg-ttl) and maybe --strict-order configurable but I don't know how
good it could be for the options. If we make some kind of default value
and make this optional this could be good however we don't want to dump
everything in the XML (if set to default for libvirt network) since we
don't want our XML to be extra-complex AFAIK (and DNSMasq is having
really many argument options).
>
>> The headers are, of course, configurable
>> in the network XML description and the idea got to me when I was reading
>> an article about DKIM (DomainKeys Identified Mail) since it's using TXT
>> records in the DNS to provide the public keys. This inspired me to
>> implement the DNS TXT record support to libvirt bridge driver to allow
>> users expose some information to the guest if they want to do so etc.
>>
>> Limitations:
>> - Records names and values containing space (' ') arguments are altered
>> to change spaces to underscores ('_'). This is because of proper
>> argument handling when spawning dnsmasq.
>
> Is this really necessary? We're not talking about a shell commandline
> here, but an array of null terminated strings. If it's a restriction
> placed by dnsmasq itself, then we should just disallow ' ' during
> parsing rather than silently changing it, to avoid surprises.
>
Well, that's the reason since if we quote this we can't use dig to dig
it correctly without the quotes. This was simply the way to disallow ' '
by changing it to underscores since I didn't want the definition to fail
because of this.
>> Technical details:
>>
>> The --txt-record argument should be supported by all version of DNSMasq
>> which allows us to use it in all of the cases for the libvirt bridge
>> driver. The only thing user has to do is to edit the network XML
>> description in libvirt and append:
>>
>> <dns>
>> <txt_record name='some name' value='some value' />
>> </dns>
>
> I was told awhile back that putting underscores in XML element names was
> strongly frowned upon (although there are certainly already examples of
> it in libvirt xml).
Well, there are some of them used in libvirt XML files and that's why I
used them. Do you think I'd rather change it not to use it and trim from
"txt_record" just to "txt" ?
> Also, it would be really nice (especially it would make Eric happy :-)
> if you included with your patch some changes to
> docs/formatnetwork.html.in to add this to the documentation.
Good point. I'll add it there once we make up some form how to have this
in the XML file and also what to make configurable by this patch, i.e.
once the patch design is having the final form.
> Have you thought about how this config model would apply to adding the
> other dns-related stuff that can be done with dnsmasq. It would be
> unfortunate if we took this first step and it turned out to not be a
> good match for the natural followons. Maybe we should take a short bit
> of time to consider the larger picture to make sure we'lll be able to
> easily and logically add the other stuff later (this might be the right
> way, I just haven't had time yet to think about it)
You're right. I completely agree since there are many DNS related
options as I'm looking into the DNSMasq man page. From what I know the
port, txt-record, query-port, edns UDP packet max-size and SRV, TXT,
PTR, NAPTR and also CNAME records are supported to be added/changed so I
guess making some larger picture of what my patch should implement could
be a good thing.
I wrote the patch to introduce just passing the TXT records to the
dnsmasq arguments however passing those information for SRV, PTR, NAPTR
and CNAME records could be a good thing as well.
This patch should be about DNS options only and if want some other
dnsmasq options that are not DNS related we may put that into a separate
patch IMHO.
What do you think about implementing these? Which do you think are good
to be implemented and which are not required to be supported?
Thanks,
Michal
--
Michal Novotny <minovotn@xxxxxxxxxx>, RHCE
Virtualization Team (xen userspace), Red Hat
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list