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

Re: [libvirt] [PATCH v2 1/8] hellolibvirt: Update hellolibvirt example



On Wed, Feb 20, 2013 at 12:38:38PM -0500, John Ferlan wrote:
> Update the code to be more in line with how code looks elsewhere in
> libvirt.  Allow listing of domains, networks, storage pools, and
> network interfaces.

I like the changes to make the style more in line with the rest of the
codebase, however, I really think that this example code should be
about a minimal use of the API to list a few domains and that's all,
so I'm not in favor of extending it to list other kinds of objects.  I
think people can find the details of how to do that kind of thing in
the API docs.

Dave

> Update the function prototypes in libvirt.c to include a message about
> the client needing to free() returned name fields.  Fix the all domains
> example flags values.
> ---
>  examples/hellolibvirt/hellolibvirt.c | 131 ++++++++++++++++++++++-------------
>  src/libvirt.c                        |  21 +++---
>  2 files changed, 91 insertions(+), 61 deletions(-)
> 
> diff --git a/examples/hellolibvirt/hellolibvirt.c b/examples/hellolibvirt/hellolibvirt.c
> index 234637e..85e5ae6 100644
> --- a/examples/hellolibvirt/hellolibvirt.c
> +++ b/examples/hellolibvirt/hellolibvirt.c
> @@ -15,7 +15,7 @@ showError(virConnectPtr conn)
>      virErrorPtr err;
>  
>      err = malloc(sizeof(*err));
> -    if (NULL == err) {
> +    if (!err) {
>          printf("Could not allocate memory for error data\n");
>          goto out;
>      }
> @@ -56,14 +56,14 @@ showHypervisorInfo(virConnectPtr conn)
>       * to fail if, for example, there is no connection to a
>       * hypervisor, so check what it returns. */
>      hvType = virConnectGetType(conn);
> -    if (NULL == hvType) {
> +    if (!hvType) {
>          ret = 1;
>          printf("Failed to get hypervisor type\n");
>          showError(conn);
>          goto out;
>      }
>  
> -    if (0 != virConnectGetVersion(conn, &hvVer)) {
> +    if (virConnectGetVersion(conn, &hvVer) < 0) {
>          ret = 1;
>          printf("Failed to get hypervisor version\n");
>          showError(conn);
> @@ -85,73 +85,101 @@ out:
>      return ret;
>  }
>  
> +/* Turn off the message in order to compile here
> + *
> + * The virConnectListAll*(), vir*IsActive(), vir*GetName(), and vir*Free()
> + * functions require specific types which aren't possible to provide in
> + * this tabular manner.
> + */
> +#pragma GCC diagnostic ignored "-Wstrict-prototypes"
> +typedef struct _objectTable objectTable;
> +struct _objectTable {
> +    const char *objName;
> +    int (*activeFcn)(virConnectPtr);
> +    int (*inactiveFcn)(virConnectPtr);
> +    int (*allFcn)();
> +    int (*isActiveFcn)();
> +    const char *(*nameFcn)();
> +    int (*freeFcn)();
> +    unsigned int flags;
> +};
> +
> +objectTable fcnTable[] = {
> +    {"domains",
> +        virConnectNumOfDomains, virConnectNumOfDefinedDomains,
> +        virConnectListAllDomains, virDomainIsActive,
> +        virDomainGetName, virDomainFree,
> +        VIR_CONNECT_LIST_DOMAINS_ACTIVE |
> +        VIR_CONNECT_LIST_DOMAINS_INACTIVE},
> +    {"networks",
> +        virConnectNumOfNetworks, virConnectNumOfDefinedNetworks,
> +        virConnectListAllNetworks, virNetworkIsActive,
> +        virNetworkGetName, virNetworkFree,
> +        VIR_CONNECT_LIST_NETWORKS_ACTIVE |
> +        VIR_CONNECT_LIST_NETWORKS_INACTIVE},
> +    {"storage pools",
> +        virConnectNumOfStoragePools, virConnectNumOfDefinedStoragePools,
> +        virConnectListAllStoragePools, virStoragePoolIsActive,
> +        virStoragePoolGetName, virStoragePoolFree,
> +        VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE |
> +        VIR_CONNECT_LIST_STORAGE_POOLS_INACTIVE},
> +    {"interfaces",
> +        virConnectNumOfInterfaces, virConnectNumOfDefinedInterfaces,
> +        virConnectListAllInterfaces, virInterfaceIsActive,
> +        virInterfaceGetName, virInterfaceFree,
> +        VIR_CONNECT_LIST_INTERFACES_ACTIVE |
> +        VIR_CONNECT_LIST_INTERFACES_INACTIVE},
> +    {NULL, NULL, NULL, NULL, NULL, NULL, NULL, 0}
> +};
>  
>  static int
> -showDomains(virConnectPtr conn)
> +showObject(virConnectPtr conn, objectTable entry)
>  {
> -    int ret = 0, i, numNames, numInactiveDomains, numActiveDomains;
> -    char **nameList = NULL;
> +    int ret = 0, numActive, numInactive, numObjects, i;
> +    void **objects = NULL;
>  
> -    numActiveDomains = virConnectNumOfDomains(conn);
> -    if (-1 == numActiveDomains) {
> +    if ((numActive = (*entry.activeFcn)(conn)) < 0) {
>          ret = 1;
> -        printf("Failed to get number of active domains\n");
> +        printf("Failed to get number of active %s\n", entry.objName);
>          showError(conn);
>          goto out;
>      }
>  
> -    numInactiveDomains = virConnectNumOfDefinedDomains(conn);
> -    if (-1 == numInactiveDomains) {
> +    if ((numInactive = (*entry.inactiveFcn)(conn)) < 0) {
>          ret = 1;
> -        printf("Failed to get number of inactive domains\n");
> +        printf("Failed to get number of inactive %s\n", entry.objName);
>          showError(conn);
>          goto out;
>      }
>  
> -    printf("There are %d active and %d inactive domains\n",
> -           numActiveDomains, numInactiveDomains);
> +    printf("There are %d active and %d inactive %s:\n",
> +           numActive, numInactive, entry.objName);
>  
> -    nameList = malloc(sizeof(*nameList) * numInactiveDomains);
> -
> -    if (NULL == nameList) {
> +    if ((numObjects = (*entry.allFcn)(conn, &objects, entry.flags)) < 0) {
>          ret = 1;
> -        printf("Could not allocate memory for list of inactive domains\n");
> -        goto out;
> -    }
> -
> -    numNames = virConnectListDefinedDomains(conn,
> -                                            nameList,
> -                                            numInactiveDomains);
> -
> -    if (-1 == numNames) {
> -        ret = 1;
> -        printf("Could not get list of defined domains from hypervisor\n");
> +        printf("Failed to get all %s\n", entry.objName);
>          showError(conn);
>          goto out;
>      }
> -
> -    if (numNames > 0) {
> -        printf("Inactive domains:\n");
> -    }
> -
> -    for (i = 0 ; i < numNames ; i++) {
> -        printf("  %s\n", *(nameList + i));
> -        /* The API documentation doesn't say so, but the names
> -         * returned by virConnectListDefinedDomains are strdup'd and
> -         * must be freed here.  */
> -        free(*(nameList + i));
> +    for (i = 0; i < numObjects; i++) {
> +        int active = (*entry.isActiveFcn)(objects[i]);
> +        /* Sure the name could be longer, but for a quick example this works */
> +        printf("\t%8s (%s)\n",
> +               (*entry.nameFcn)(objects[i]),
> +               (active == 1 ? "active" : "non-active"));
> +
> +        /* We have to free the entry */
> +        (*entry.freeFcn)(objects[i]);
>      }
> -
> +    free(objects);
>  out:
> -    free(nameList);
>      return ret;
>  }
>  
> -
>  int
>  main(int argc, char *argv[])
>  {
> -    int ret = 0;
> +    int ret = 0, i;
>      virConnectPtr conn;
>      char *uri;
>  
> @@ -163,7 +191,7 @@ main(int argc, char *argv[])
>       * except, possibly, the URI of the hypervisor. */
>      conn = virConnectOpenAuth(uri, virConnectAuthPtrDefault, 0);
>  
> -    if (NULL == conn) {
> +    if (!conn) {
>          ret = 1;
>          printf("No connection to hypervisor\n");
>          showError(conn);
> @@ -171,7 +199,7 @@ main(int argc, char *argv[])
>      }
>  
>      uri = virConnectGetURI(conn);
> -    if (NULL == uri) {
> +    if (!uri) {
>          ret = 1;
>          printf("Failed to get URI for hypervisor connection\n");
>          showError(conn);
> @@ -181,18 +209,21 @@ main(int argc, char *argv[])
>      printf("Connected to hypervisor at \"%s\"\n", uri);
>      free(uri);
>  
> -    if (0 != showHypervisorInfo(conn)) {
> +    if (showHypervisorInfo(conn) < 0) {
>          ret = 1;
>          goto disconnect;
>      }
>  
> -    if (0 != showDomains(conn)) {
> -        ret = 1;
> -        goto disconnect;
> +    i = 0;
> +    while (fcnTable[i].objName) {
> +        ret = showObject(conn, fcnTable[i]);
> +        if (ret != 0)
> +            goto disconnect;
> +        i++;
>      }
>  
>  disconnect:
> -    if (0 != virConnectClose(conn)) {
> +    if (virConnectClose(conn) < 0) {
>          printf("Failed to disconnect from hypervisor\n");
>          showError(conn);
>          ret = 1;
> diff --git a/src/libvirt.c b/src/libvirt.c
> index 1e78500..e717f16 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -8304,19 +8304,18 @@ error:
>   * VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT, for filtering based on whether
>   * a domain has snapshots.
>   *
> - * Returns the number of domains found or -1 and sets domains to NULL in case
> - * of error.  On success, the array stored into @doms is guaranteed to have an
> + * Returns the number of domains found or -1 and sets domains to NULL in case of
> + * error.  On success, the array stored into @domains is guaranteed to have an
>   * extra allocated element set to NULL but not included in the return count, to
>   * make iteration easier. The caller is responsible for calling virDomainFree()
> - * on each array element, then calling free() on @doms.
> + * on each array element, then calling free() on @domains.
>   *
>   * Example of usage:
>   * virDomainPtr *domains;
> - * virDomainPtr dom;
>   * int i;
>   * int ret;
> - * unsigned int flags = VIR_CONNECT_LIST_RUNNING |
> - *                      VIR_CONNECT_LIST_PERSISTENT;
> + * unsigned int flags = VIR_CONNECT_LIST_DOMAINS_RUNNING |
> + *                      VIR_CONNECT_LIST_DOMAINS_PERSISTENT;
>   *
>   * ret = virConnectListAllDomains(conn, &domains, flags);
>   * if (ret < 0)
> @@ -10226,7 +10225,7 @@ error:
>   * this command is inherently racy; a network can be started between a call
>   * to virConnectNumOfNetworks() and this call; you are only guaranteed that
>   * all currently active networks were listed if the return is less than
> - * @maxnames.
> + * @maxnames. The client must call free() on each returned name.
>   */
>  int
>  virConnectListNetworks(virConnectPtr conn, char **const names, int maxnames)
> @@ -11208,7 +11207,7 @@ error:
>   * this command is inherently racy; a interface can be started between a call
>   * to virConnectNumOfInterfaces() and this call; you are only guaranteed that
>   * all currently active interfaces were listed if the return is less than
> - * @maxnames.
> + * @maxnames. The client must call free() on each returned name.
>   */
>  int
>  virConnectListInterfaces(virConnectPtr conn, char **const names, int maxnames)
> @@ -12076,7 +12075,7 @@ error:
>   * this command is inherently racy; a pool can be started between a call to
>   * virConnectNumOfStoragePools() and this call; you are only guaranteed
>   * that all currently active pools were listed if the return is less than
> - * @maxnames.
> + * @maxnames. The client must call free() on each returned name.
>   */
>  int
>  virConnectListStoragePools(virConnectPtr conn,
> @@ -18285,7 +18284,7 @@ error:
>   * the results, see virDomainListAllSnapshots().
>   *
>   * Returns the number of domain snapshots found or -1 in case of error.
> - * The caller is responsible for freeing each member of the array.
> + * The caller is responsible to call free() for each member of the array.
>   */
>  int
>  virDomainSnapshotListNames(virDomainPtr domain, char **names, int nameslen,
> @@ -18534,7 +18533,7 @@ error:
>   * the results, see virDomainSnapshotListAllChildren().
>   *
>   * Returns the number of domain snapshots found or -1 in case of error.
> - * The caller is responsible for freeing each member of the array.
> + * The caller is responsible to call free() for each member of the array.
>   */
>  int
>  virDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot,
> -- 
> 1.7.11.7
> 
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list


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