[libvirt] [PATCHv5 1/5] domifaddr: Implement the public APIs

[Osier Yang]

On 04/09/13 03:07, Eric Blake wrote:
> On 09/01/2013 07:43 AM, Nehal J Wani wrote:
>> Define a new API virDomainInterfaceAddresses, which returns
>> the address information of a running domain's interfaces(s).
>> If no interface name is specified, it returns the information
>> of all interfaces, otherwise it only returns the information
>> of the specificed interface. The address information includes
>> the MAC and IP addresses.
>>
>> Define helper function virDomainInterfaceFree, which allows
>> the upper layer application to free the domain interface object
>> conveniently.
>>
>> The API is going to provide multiple methods by flags, e.g.
>>
>>    * Query guest agent
>>    * Parse lease file of dnsmasq
>>    * DHCP snooping
>>
>> But at this stage, it will only work with guest agent, and flags
>> won't be supported.
> That worries me a bit.  Ultimately, we want our interfaces to behave as
> sane as possible when flags==0; rather than making the behavior be that
> 'flags==0' implies 'only guest agent probe', I'd rather introduce a flag
> right away up front that says 'include guest agent probe in the set of
> attempted methods', and then document that 'flags==0 is shorthand for
> letting the hypervisor choose the best method(s) out of supported
> possibilities)'.  In other words, I want to make sure that this API will
> be similar to virDomainShutdownFlags, where a flags of 0 lets the
> hypervisor choose between methods, a single explicit flag forces the
> hypervisor to use that method alone, and more than one flag can be OR'd
> together to let the hypervisor choose among that subset of flags.

I can't find any reason to say this is not good. Except that I'm wondering
which the upper layer developer prefer more, the more straightforward
style we used for the flags of old APIs (flags==0 defaults to a specific
flag), or the style like this (flags==0 defaults to letting the hypervisor
choosing it, and even choosing between subset of the OR'ed flags)?


>
>> +    char *addr;              /* IP address */
>> +    unsigned int prefix;     /* IP address prefix */
> Do we ever intend to support non-CIDR IPv4 addresses (where the mask is
> not contiguous bits)?  Or are we intentionally documenting that the
> prefix will always be possible because we always require the mask to be
> a contiguous prefix?

We don't have the way to require it, it's returned either from guest
agent, leases file, and traffic snooping, unless we reject if they
returns non-contiguous bits for the subnet mask, but that doesn't
make sense.

So I'm wondering if we should introduce a new member for the netmask,
though qemu guest agent doesn't return anything for the netmask
(I guess it always assumes the netmask bits are contiguous), for
the other 2 methods, we still might faces the non-contiguous netmask
bits.

>
>> @@ -1238,6 +1243,7 @@ struct _virDriver {
>>       virDrvDomainInterfaceStats domainInterfaceStats;
>>       virDrvDomainSetInterfaceParameters domainSetInterfaceParameters;
>>       virDrvDomainGetInterfaceParameters domainGetInterfaceParameters;
>> +    virDrvDomainInterfaceAddresses    domainInterfaceAddresses;
> Spacing is off; only one space needed.
>
>> + *
>> + * cleanup:
>> + *     if (ifaces)
>> + *         for (i = 0; i < ifaces_count; i++)
>> + *             virDomainInterfaceFree(ifaces[i]);
>> + *     VIR_FREE(ifaces);
> VIR_FREE() is not a public function; this comment should instead mention
> free(); because it is in a comment, it will not trigger our syntax check
> rules.
>
>> +int
>> +virDomainInterfaceAddresses(virDomainPtr dom,
>> +                            virDomainInterfacePtr **ifaces,
>> +                            unsigned int flags)
>> +{
>> +    virConnectPtr conn;
>> +
>> +    VIR_DOMAIN_DEBUG(dom, "ifaces=%p, flags=%x", ifaces, flags);
>> +
>> +    virResetLastError();
>> +
>> +    if (!VIR_IS_CONNECTED_DOMAIN(dom)) {
>> +        virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>> +        virDispatchError(NULL);
>> +        return -1;
>> +    }
>> +
>> +    conn = dom->conn;
> Putting on my security hat - anything that requires guest interaction
> should probably not be permitted from a read-only connection (because a
> malicious guest coupled with a read-only caller purposefully exploiting
> the guest's intentional bad behavior might open up a denial of service
> against read-write clients).  Again, all the more reason why I think you
> should require non-zero flags before permitting guest agent interaction,
> so that we can then add a security check here (if you pass flags=0, you
> get the default set of actions that are safe for a read-only client; but
> if you pass flags=..._AGENT, then we can prevent the call from
> succeeding on a read-only client).
>
> Overall, I'm happy with what we finally settled on for the API, and my
> questions now only involve whether we need a non-zero flag before
> allowing agent interaction.
>