[libvirt] [PATCHv3 4/4] net-dhcp-leases: Add virsh support

[Eric Blake]

On 09/15/2013 11:49 PM, Nehal J Wani wrote:
> Use virNetworkGetDHCPLeases and virNetworkGetDHCPLeasesForMAC in virsh.
> 
> The new feature supports the follwing methods:
> 
> 1. Retrieve leases info for a given virtual network
> 
> 2. Retrieve leases info for given network interface
> 

> +
> +static bool
> +cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd *cmd)
> +{
> +    const char *name = NULL;
> +    const char *mac = NULL;
> +    virNetworkDHCPLeasesPtr *leases = NULL;
> +    int nleases = 0;
> +    bool ret = false;
> +    size_t i;
> +    unsigned int flags = 0;
> +    virNetworkPtr network = NULL;
> +
> +    if (vshCommandOptString(cmd, "mac", &mac) < 0)
> +        return false;
> +
> +    if (!(network = vshCommandOptNetworkBy(ctl, cmd, &name,
> +                                           VSH_BYNAME | VSH_BYUUID)))
> +        return false;

Just use vshCommandOptNetwork (no need to use the filtering of NetworkBy).

> +
> +    nleases = mac ? virNetworkGetDHCPLeasesForMAC(network, mac, &leases, flags)
> +        : virNetworkGetDHCPLeases(network, &leases, flags);
> +
> +    if (nleases < 0) {
> +        vshError(ctl, _("Failed to get leases info for %s"), name);
> +        goto cleanup;
> +    }
> +
> +    vshPrintExtra(ctl, " %-20s %-20s %-10s %-20s %-12s %s\n%s%s\n",
> +                  _("Expiry Time"), _("MAC address"), _("Protocol"),
> +                  _("IP address"), _("Hostname"), _("ClientId"),
> +                  "------------------------------------------------",
> +                  "------------------------------------------------");
> +
> +    for (i = 0; i < nleases; i++) {

Should you qsort() the list before printing it, to ensure that identical
MAC lines are grouped even if the driver didn't hand them back in any
particular order?

> +        const char *type = NULL;
> +        virNetworkDHCPLeasesPtr lease = leases[i];
> +        time_t expirytime_tmp = lease->expirytime;
> +        struct tm ts;
> +        char expirytime[32];
> +        ts = *localtime_r(&expirytime_tmp, &ts);
> +        strftime(expirytime, sizeof(expirytime), "%d-%m-%Y %H:%M:%S", &ts);

Please use ISO %Y-%m-%d, as it is sortable, and also has the benefit of
being unambiguous in both US and UK.

> +
> +        switch (lease->type) {
> +        case VIR_IP_ADDR_TYPE_IPV4:
> +            type = "ipv4";
> +            break;
> +        case VIR_IP_ADDR_TYPE_IPV6:
> +            type = "ipv6";
> +            break;
> +        }
> +
> +        vshPrintExtra(ctl, " %-20s %-20s %-10s %s/%-5d %-12s %s\n",
> +                      expirytime, lease->mac, type, lease->ipaddr,

If a third lease->type is introduced in a future libvirtd, then you risk
passing NULL to printf(%s).  Please use NULLSTR(type), so that you don't
crash on non-glibc systems.

> +                      lease->prefix, lease->hostname, lease->clientid);

Do we guarantee that ALL string fields of lease are populated with
non-NULL strings (even if the empty string), no matter what?  Or should
we revisit the docs in 1/4, and the RPC implementation of 2/4, to allow
for NULL strings for fields that aren't available?

> +    }
> +
> +    ret = true;
> +
> +cleanup:
> +    if (leases) {
> +        for (i = 0; i < nleases; i++)
> +            virNetworkDHCPLeaseFree(leases[i]);
> +    }
> +    VIR_FREE(leases);
> +    virNetworkFree(network);
> +    return ret;
> +}
>  
>  const vshCmdDef networkCmds[] = {
>      {.name = "net-autostart",
> @@ -1209,5 +1303,11 @@ const vshCmdDef networkCmds[] = {
>       .info = info_network_uuid,
>       .flags = 0
>      },
> +    {.name = "net-dhcp-leases",
> +     .handler = cmdNetworkDHCPLeases,
> +     .opts = opts_network_dhcp_leases,
> +     .info = info_network_dhcp_leases,
> +     .flags = 0

Not your fault, but we should be using trailing commas when doing C99
struct initialization.

> +    },
>      {.name = NULL}
>  };
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 0ae5178..b87f646 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -2325,6 +2325,12 @@ If I<--current> is specified, affect the current network state.
>  Both I<--live> and I<--config> flags may be given, but I<--current> is
>  exclusive. Not specifying any flag is the same as specifying I<--current>.
>  
> +=item B<net-dhcp-leases> I<network> I<mac>

List it as '[I<mac>]', to make it obvious the mac address is optional.

> +
> +Get a list of dhcp leases for all network interfaces connected to the given
> +virtual I<network> or limited output just for one interface if I<mac> is
> +specified.
> +
>  =back
>  
>  =head1 INTERFACE COMMANDS
> 

-- 
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/20130916/991b1895/attachment-0001.sig>