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

[Eric Blake]

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.

> +    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?

> @@ -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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130903/f39e38ab/attachment-0001.sig>