[libvirt] [PATCH 2/3] network: allow disabling dnsmasq's DNS server

[Laine Stump]

On 08/19/2016 02:50 AM, Michal Privoznik wrote:
> On 18.08.2016 20:42, Laine Stump wrote:
>> On Aug 18, 2016 5:01 AM, "Michal Privoznik" <mprivozn at redhat.com> wrote:
>>> On 12.08.2016 04:41, Laine Stump wrote:
>>>>             def->nsrvs || def->ntxts))
>>>>           return 0;
>>>>
>>>>       virBufferAddLit(buf, "<dns");
>>>> -    /* default to "yes", but don't format it in the XML */
>>>> +    if (def->enable) {
>>>> +        const char *fwd = virTristateBoolTypeToString(def->enable);
>>>> +
>>>> +        if (!fwd) {
>>>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>>>> +                           _("Unknown enable type %d in network"),
>>>> +                           def->enable);
>>>> +            return -1;
>>> I don't think check is needed. We've validated the forward mode when
>>> parsing the XML.
>>>
>> Depends on your philosophy. If you trust that nothing could have modified
>> the value from the time it was parsed until the time it's formated, then
>> yes. If you don't trust that (or are uncomfortable that a pointer from a
>> function that could potentially return NULL is used as an argument to an
>> sprintf-like function without checking for NULL), then no.
> So I've just checked and the worst case scenario is that we produce "
> enable='(null)'" into the XML (without any crash),

Yes, as long as you are using glibc's sprintf, which checks for a NULL 
argument being used for%s and replaces it with a pointer to "(null)". As 
far as I know this isn't guaranteed by any standard though, and libvirt 
is built on platforms that don't use glibc.

>   which to me is a risk
> I can live with.

If all platforms used (or behaved the same as) glibc, then I would agree 
(although having the error log there would make it easier to find any 
future bugs).

>   Moreover, if the value has been modified, we can't be
> entirely sure it was modified to something outside boundaries. It might
> as well be changed from 'no' to 'yes' (or vice versa) which is not any
> worse than the previous case IMO.

I don't follow the chain of logic there.