Re: [libvirt] [PATCH 2/3] network: allow disabling dnsmasq's DNS server
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(a)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.