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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list