Re: [PATCH v9 1/4] domifaddr: Implement the public APIs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 03/09/2015 09:09 AM, Daniel P. Berrange wrote:
> From: Nehal J Wani <nehaljw.kkd1@xxxxxxxxx>
> 
> Define helper function virDomainInterfaceFree, which allows
> the upper layer application to free the domain interface object
> conveniently.
> 

>  
> +typedef enum {
> +    VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LEASE = 0, /* Parse DHCP lease file */
> +    VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT = 1, /* Query qemu guest agent */
> +} virDomainInterfaceAddressesSource;

Missing a conditional _LAST member.

>  }
> +
> +/**
> + * virDomainInterfaceAddresses:
> + * @dom: domain object
> + * @ifaces: pointer to an array of pointers pointing to interface objects
> + * @source: one of the virDomainInterfaceAddressesSource constants
> + * @flags: currently unused, pass zero
> + *
> + * Return a pointer to the allocated array of pointers pointing to interfaces

s/pointers pointing/pointers/

> + * present in given domain along with their IP and MAC addresses. Note that
> + * single interface can have multiple or even 0 IP address.

s/address/addresses/

> + *
> + * This API dynamically allocates the virDomainInterfacePtr struct based on
> + * how many interfaces domain @dom has, usually there's 1:1 correlation. The
> + * count of the interfaces is returned as the return value.
> + *
> + * If @source is VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LEASE, the DHCP lease
> + * file associated with any virtual networks will be examined to obtain
> + * the interface addresses. This only returns data for interfaces which
> + * are connected to virtual networks managed by libvirt.
> + *
> + * If @source is VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT, a configured
> + * guest agent is needed for successful return from this API. Moreover, if
> + * guest agent is used then the interface name is the one seen by guest OS.
> + * To match such interface with the one from @dom XML use MAC address or IP
> + * range.
> + *
> + * @ifaces->name and @ifaces->hwaddr are never NULL.

[1]

> + *
> + * The caller *must* free @ifaces when no longer needed. Usual use case
> + * looks like this:
> + *
> + * virDomainInterfacePtr *ifaces = NULL;
> + * int ifaces_count = 0;

Add more indentation before the example, so that the documentation
formatter will render it correctly (if I recall correctly, just a single
additional space would suffice).

> + * size_t i, j;
> + * virDomainPtr dom = ... obtain a domain here ...;
> + *
> + * if ((ifaces_count = virDomainInterfaceAddresses(dom, &ifaces,
> + *           VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LEASE)) < 0)
> + *     goto cleanup;
> + *
> + * ... do something with returned values, for example:
> + * for (i = 0; i < ifaces_count; i++) {
> + *     printf("name: %s", ifaces[i]->name);
> + *     if (ifaces[i]->hwaddr)

This is dead code, according to the claim in [1] that hwaddr is never
NULL (or this is right, but the docs are wrong).

> + *         printf(" hwaddr: %s", ifaces[i]->hwaddr);
> + *
> + *     for (j = 0; j < ifaces[i]->naddrs; j++) {
> + *         virDomainIPAddressPtr ip_addr = ifaces[i]->addrs + j;
> + *         printf("[addr: %s prefix: %d type: %d]",
> + *                ip_addr->addr, ip_addr->prefix, ip_addr->type);
> + *     }
> + *     printf("\n");
> + * }
> + *
> + * cleanup:
> + *     if (ifaces)
> + *         for (i = 0; i < ifaces_count; i++)
> + *             virDomainInterfaceFree(ifaces[i]);
> + *     free(ifaces);
> + *
> + * Returns the number of interfaces on success, -1 in case of error.
> + */
> +int
> +virDomainInterfaceAddresses(virDomainPtr dom,
> +                            virDomainInterfacePtr **ifaces,
> +                            unsigned int source,
> +                            unsigned int flags)
> +{
> +    virConnectPtr conn;
> +
> +    VIR_DOMAIN_DEBUG(dom, "ifaces=%p, source=%d, flags=%x", ifaces, source, flags);
> +
> +    virResetLastError();
> +
> +    conn = dom->conn;
> +
> +    virCheckDomainReturn(dom, -1);
> +    virCheckNonNullArgGoto(ifaces, error);
> +    virCheckReadOnlyGoto(conn->flags, error);

If we fail this check...

> +
> +    *ifaces = NULL;

...then the caller loses out on the guarantee that *ifaces was set to
NULL.  Is that going to be a problem?  Elsewhere, such as in
virConnectListAllDomains, we explicitly do

if (domains)
    *domains = NULL;
virCheckConnectReturn(conn, -1);

to guarantee that all early-exit paths set a valid non-null pointer to NULL.

> +/**
> + * virDomainInterfaceFree:
> + * @iface: an interface object
> + *
> + * Free the interface object. The data structure is
> + * freed and should not be used thereafter.
> + */
> +void
> +virDomainInterfaceFree(virDomainInterfacePtr iface)
> +{
> +    size_t i;
> +
> +    if (!iface)
> +        return;

I'd document that this one is explicitly safe to call on NULL (since
some of our older API return failure on attempt to "Free" a NULL pointer).

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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]