[libvirt] [PATCH 2/2] Network: Add support for DNS hosts definition

[Michal Novotny]

On 03/31/2011 10:16 AM, Paolo Bonzini wrote:
>> +<zeroOrMore>
>> +<element name="host">
>> +<attribute name="ip"><ref name="ipv4-addr"/></attribute>
>> +<zeroOrMore>
>> +<element name="hostname"><text/></element>
>> +</zeroOrMore>
> oneOrMore hostname elements seems preferrable.


Well, this could be better. That's right.


>> +    if ((fd = open(hostsFile,
> You can use fopen and fprintf.



Well, I don't know since it's rarely used in to fopen and fprintf form
and I'm not sure whether syntax checking enables it or not. I know there
are some native functions disabled so this one can be as well but open()
is not disabled since it's used somewhere - that's what I'm sure about.



> This function also should be in src/util/dnsmasq.c (e.g. with addnhost* 
> APIs in that file and dnsmasqAddHost as a public API).  See the 
> hostsfile* APIs there.

Oh, I'll have a look to this one.

> Then you could further split the patch this way:
>
> - one patch that moves the creation of the dnsmasqContext into 
> networkSaveDnsmasqHostsfile;


What do you mean by dnsmasqContents? The code to generate the hostsfile,
i.e. the one I added?


> - one patch that makes src/util/dnsmasq.c create the hostsfile only 
> after the first call to dnsmasqAddDhcpHost


You mean that the first call to dnsmasqAddDhcpHost should call the
function you mentioned about? Also, this is named dnsmasqAddDhcpHost()
.. what if somebody will use <dns> tag but without <dhcp> tag?


> - this would be the third patch in the series, and it would add 
> dnsmasqAddHost calls in networkSaveDnsmasqHostsfile

So this patch will be just adding the networkSaveDnsmasqHostsfile() call
to the dnsmasqAddHost() and nothing else ?

>> diff --git a/tests/networkxml2xmlin/nat-network-dns-hosts.xml b/tests/networkxml2xmlin/nat-network-dns-hosts.xml
>> new file mode 100644
>> index 0000000..fe545cf
> Uhm, libvirt has no tests that actually check whether the dnsmasq 
> command line works?  That's a bit bad, perhaps you can add them...

Well, unfortunately there are no tests to check whether the dnsmasq
command line works. What would you prefer? To run it with some bogus PID
file and port and check the command execution error code and if it's 0
(and PID is working) then to kill $PID and make the test pass, otherwise
fail the test or something similar?

Michal

-- 
Michal Novotny <minovotn at redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat