[PATCH v3 00/10] xl: improve coding style and return more failure on

[PATCH v3 00/10] xl: improve coding style and return more failure on

[Paulina Szubarczyk]

This is my bite-sized outreachy project [1][2]. 

The patch aims to improve coding_style and return failure for more xl commands:
- pci-*
-- tmem-*

After rebase to staging it seems that the patch {09} cleaning libxl_set_memory_target() 
to return useful error codes from [0] is not applied I resent it here.

In pci-* functions libxl__create_pci_backend(), libxl__device_pci_destroy_all()
error codes are returned instead of always 0.

Int the changes I follow CODING_STYLE as in the coresponding patches:
 - In general, don't initialize rc / r, but use set-and-goto
 - Use 'r' for non-libxl error codes
 - Use EXIT_FAILURE and EXIT_SUCCESS in main_foo()
 - Use 1 and 0 in internal functions of xl

[0] http://lists.xenproject.org/archives/html/xen-devel/2015-12/msg02246.html
[1] http://lists.xenproject.org/archives/html/xen-devel/2016-03/msg03031.html
[2] https://www.mail-archive.com/xen-devel@lists.xen.org/msg62055.html

Changed since v1:
 * Make  libxl__device_from_pcidev() return void instead 0.
 * Modify the libxl_device_pci_assignable_list() function to use
   only one 'out' cleaning path.
 * Changed exit() calls to 'return 1;' for set_memory_* and pci_* functions
   in xl_cmdimpl 
 * Added the error cleanup path 'out' in libxl_tmem_* functions and replaced 
   return with set-and-goto statements.
 * Use 'lrc' for return values to local functions libxl__*
 
 Changed since v2:
 * Revert the changes of 'exit() -> return 1;' since other patches take care of 
   that
 * Improve changelog messages 
 * Change in logging of libxc error 'LOGEV() -> LOG();  

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v3 1/7] libxl: improve return codes for some pci related functions

[Paulina Szubarczyk]

*libxl__device_from_pcidev() initialize the values of libxl__device
 struct and can be void.

*libxl__create_pci_backend(), libxl__device_pci_destroy_all()
 should propagate the success/error, rather than always returning 0.

Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
---
Changes since v2:
  - changed the changelog as indicated by Dario.
---
 tools/libxl/libxl_pci.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 300fd4d..b4be967 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -64,7 +64,7 @@ static void libxl_create_pci_backend_device(libxl__gc *gc, flexarray_t *back, in
     flexarray_append_pair(back, GCSPRINTF("state-%d", num), GCSPRINTF("%d", XenbusStateInitialising));
 }
 
-static int libxl__device_from_pcidev(libxl__gc *gc, uint32_t domid,
+static void libxl__device_from_pcidev(libxl__gc *gc, uint32_t domid,
                                      libxl_device_pci *pcidev,
                                      libxl__device *device)
 {
@@ -74,8 +74,6 @@ static int libxl__device_from_pcidev(libxl__gc *gc, uint32_t domid,
     device->devid = 0;
     device->domid = domid;
     device->kind = LIBXL__DEVICE_KIND_PCI;
-
-    return 0;
 }
 
 int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid,
@@ -84,13 +82,11 @@ int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid,
     flexarray_t *front = NULL;
     flexarray_t *back = NULL;
     libxl__device device;
-    int ret = ERROR_NOMEM, i;
+    int i;
 
     front = flexarray_make(gc, 16, 1);
     back = flexarray_make(gc, 16, 1);
 
-    ret = 0;
-
     LOG(DEBUG, "Creating pci backend");
 
     /* add pci device */
@@ -108,12 +104,10 @@ int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid,
     flexarray_append_pair(front, "backend-id", GCSPRINTF("%d", 0));
     flexarray_append_pair(front, "state", GCSPRINTF("%d", XenbusStateInitialising));
 
-    libxl__device_generic_add(gc, XBT_NULL, &device,
-                              libxl__xs_kvs_of_flexarray(gc, back, back->count),
-                              libxl__xs_kvs_of_flexarray(gc, front, front->count),
-                              NULL);
-
-    return 0;
+    return libxl__device_generic_add(gc, XBT_NULL, &device,
+                                     libxl__xs_kvs_of_flexarray(gc, back, back->count),
+                                     libxl__xs_kvs_of_flexarray(gc, front, front->count),
+                                     NULL);
 }
 
 static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, int starting)
@@ -1612,7 +1606,7 @@ int libxl__device_pci_destroy_all(libxl__gc *gc, uint32_t domid)
     }
 
     free(pcidevs);
-    return 0;
+    return rc;
 }
 
 int libxl__grant_vga_iomem_permission(libxl__gc *gc, const uint32_t domid,
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 1/7] libxl: improve return codes for some pci related functions

[Olaf Hering]

On Wed, Apr 20, Paulina Szubarczyk wrote:

> *libxl__device_from_pcidev() initialize the values of libxl__device
>  struct and can be void.

> -static int libxl__device_from_pcidev(libxl__gc *gc, uint32_t domid,
> +static void libxl__device_from_pcidev(libxl__gc *gc, uint32_t domid,
>                                       libxl_device_pci *pcidev,
>                                       libxl__device *device)
>  {
> @@ -74,8 +74,6 @@ static int libxl__device_from_pcidev(libxl__gc *gc, uint32_t domid,
>      device->devid = 0;
>      device->domid = domid;
>      device->kind = LIBXL__DEVICE_KIND_PCI;
> -
> -    return 0;

If you dont have it on your radar already:
This should be done for all other functions of that type as well. I was
always wondering why they had to return something.

Olaf

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 1/7] libxl: improve return codes for some pci related functions

[Wei Liu]

Apart from what Oalf already said, I have one minor below.

On Wed, Apr 20, 2016 at 10:03:58AM +0200, Paulina Szubarczyk wrote:
> *libxl__device_from_pcidev() initialize the values of libxl__device
>  struct and can be void.
> 
> *libxl__create_pci_backend(), libxl__device_pci_destroy_all()
>  should propagate the success/error, rather than always returning 0.
> 
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
> ---
> Changes since v2:
>   - changed the changelog as indicated by Dario.
> ---
>  tools/libxl/libxl_pci.c | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> index 300fd4d..b4be967 100644
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -64,7 +64,7 @@ static void libxl_create_pci_backend_device(libxl__gc *gc, flexarray_t *back, in
>      flexarray_append_pair(back, GCSPRINTF("state-%d", num), GCSPRINTF("%d", XenbusStateInitialising));
>  }
>  
> -static int libxl__device_from_pcidev(libxl__gc *gc, uint32_t domid,
> +static void libxl__device_from_pcidev(libxl__gc *gc, uint32_t domid,
>                                       libxl_device_pci *pcidev,
>                                       libxl__device *device)

Could you also fix the indentation, please?

Wei.
 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v3 2/7] libxl: style cleanups in libxl_device_pci_assignable_list()

[Paulina Szubarczyk]

Various coding style compliance cleanups, such as, arranging for
using only one path out of the function, whitespaces in loops ad if-s
and r instead of rc for storing non-libxl error codes.

Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
---
Changes since v2:
 - changed the changelog
---
 tools/libxl/libxl_pci.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index b4be967..6ac0a92 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -398,34 +398,34 @@ libxl_device_pci *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num)
     libxl_device_pci *pcidevs = NULL, *new, *assigned;
     struct dirent *de;
     DIR *dir;
-    int rc, num_assigned;
+    int r, num_assigned;
 
     *num = 0;
 
-    rc = get_all_assigned_devices(gc, &assigned, &num_assigned);
-    if ( rc )
-        goto out;
+    r = get_all_assigned_devices(gc, &assigned, &num_assigned);
+    if (r) goto out;
 
     dir = opendir(SYSFS_PCIBACK_DRIVER);
-    if ( NULL == dir ) {
-        if ( errno == ENOENT ) {
+    if (NULL == dir) {
+        if (errno == ENOENT) {
             LOG(ERROR, "Looks like pciback driver not loaded");
-        }else{
+        } else {
             LOGE(ERROR, "Couldn't open %s", SYSFS_PCIBACK_DRIVER);
         }
-        goto out_closedir;
+        closedir(dir);
+        goto out;
     }
 
-    while( (de = readdir(dir)) ) {
+    while((de = readdir(dir))) {
         unsigned dom, bus, dev, func;
-        if ( sscanf(de->d_name, PCI_BDF, &dom, &bus, &dev, &func) != 4 )
+        if (sscanf(de->d_name, PCI_BDF, &dom, &bus, &dev, &func) != 4)
             continue;
 
-        if ( is_pcidev_in_array(assigned, num_assigned, dom, bus, dev, func) )
+        if (is_pcidev_in_array(assigned, num_assigned, dom, bus, dev, func))
             continue;
 
         new = realloc(pcidevs, ((*num) + 1) * sizeof(*new));
-        if ( NULL == new )
+        if (NULL == new)
             continue;
 
         pcidevs = new;
@@ -436,8 +436,6 @@ libxl_device_pci *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num)
         (*num)++;
     }
 
-out_closedir:
-    closedir(dir);
 out:
     GC_FREE;
     return pcidevs;
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 2/7] libxl: style cleanups in libxl_device_pci_assignable_list()

[Olaf Hering]

On Wed, Apr 20, Paulina Szubarczyk wrote:

> @@ -398,34 +398,34 @@ libxl_device_pci *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num)
>      dir = opendir(SYSFS_PCIBACK_DRIVER);

> +    while((de = readdir(dir))) {

> -    closedir(dir);

This change seems to leak 'dir' in success case.

Olaf

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 2/7] libxl: style cleanups in libxl_device_pci_assignable_list()

[Paulina Szubarczyk]

On Wed, 2016-04-20 at 22:24 +0200, Olaf Hering wrote:
> On Wed, Apr 20, Paulina Szubarczyk wrote:
> 
> > @@ -398,34 +398,34 @@ libxl_device_pci *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num)
> >      dir = opendir(SYSFS_PCIBACK_DRIVER);
> 
> > +    while((de = readdir(dir))) {
> 
> > -    closedir(dir);
> 
> This change seems to leak 'dir' in success case.

I am sorry, that is true. 

The previous out_closedir path was run from the path after the
condition:
+    if (NULL == dir) {

-        goto out_closedir;
+        closedir(dir);
+        goto out;
     }

-out_closedir:
-    closedir(dir);
 out:
     GC_FREE;
     return pcidevs;

so in this case it could just jump to goto out without calling
closedir(NULL). 

Paulina

> Olaf



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v3 3/7] xl_cmdimpl: improve return codes for cd-insert commands

[Paulina Szubarczyk]

 - Use EXIT_{SUCCESS,FAILURE} for main_cd*() function
 - Use 0/1 as return values of cd_insert function

Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 6346017..815b9de 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3479,16 +3479,16 @@ static int cd_insert(uint32_t domid, const char *virtdev, char *phys)
         && stat(disk.pdev_path, &b)) {
         fprintf(stderr, "Cannot stat file: %s\n",
                 disk.pdev_path);
-        r = EXIT_FAILURE;
+        r = 1;
         goto out;
     }
 
     if (libxl_cdrom_insert(ctx, domid, &disk, NULL)) {
-        r = EXIT_FAILURE;
+        r = 1;
         goto out;
     }
 
-    r = EXIT_SUCCESS;
+    r = 0;
 
 out:
     libxl_device_disk_dispose(&disk);
@@ -3510,7 +3510,10 @@ int main_cd_eject(int argc, char **argv)
     domid = find_domain(argv[optind]);
     virtdev = argv[optind + 1];
 
-    return cd_insert(domid, virtdev, NULL);
+    if (cd_insert(domid, virtdev, NULL))
+        return EXIT_FAILURE;
+
+    return EXIT_SUCCESS;
 }
 
 int main_cd_insert(int argc, char **argv)
@@ -3528,7 +3531,10 @@ int main_cd_insert(int argc, char **argv)
     virtdev = argv[optind + 1];
     file = argv[optind + 2];
 
-    return cd_insert(domid, virtdev, file);
+    if (cd_insert(domid, virtdev, file))
+        return EXIT_FAILURE;
+
+    return EXIT_SUCCESS;
 }
 
 int main_usbctrl_attach(int argc, char **argv)
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 3/7] xl_cmdimpl: improve return codes for cd-insert commands

[Olaf Hering]

On Wed, Apr 20, Paulina Szubarczyk wrote:

>  - Use EXIT_{SUCCESS,FAILURE} for main_cd*() function
>  - Use 0/1 as return values of cd_insert function
> 
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Olaf Hering <olaf@aepfle.de>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v3 4/7] xl_cmdimpl - Add return codes for pci-detach, pci-attach, pci-asssignable-add, and pci-assignable-remove.

[Paulina Szubarczyk]

Returning error codes makes it easier for shell scripts to tell if a
command has failed or succeeded.

Signed-off-by: George Dunlap <George.Dunlap@eu.citrix.com>
Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
---
Changed since v2:
 - Remove the change to exit code since the other pathes take care of that
 
---
 tools/libxl/xl_cmdimpl.c | 68 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 49 insertions(+), 19 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 815b9de..6bef201 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3814,10 +3814,11 @@ int main_pcilist(int argc, char **argv)
     return 0;
 }
 
-static void pcidetach(uint32_t domid, const char *bdf, int force)
+static int pcidetach(uint32_t domid, const char *bdf, int force)
 {
     libxl_device_pci pcidev;
     XLU_Config *config;
+    int r = 0;
 
     libxl_device_pci_init(&pcidev);
 
@@ -3828,13 +3829,18 @@ static void pcidetach(uint32_t domid, const char *bdf, int force)
         fprintf(stderr, "pci-detach: malformed BDF specification \"%s\"\n", bdf);
         exit(2);
     }
-    if (force)
-        libxl_device_pci_destroy(ctx, domid, &pcidev, 0);
-    else
-        libxl_device_pci_remove(ctx, domid, &pcidev, 0);
+    if (force) {
+        if (libxl_device_pci_destroy(ctx, domid, &pcidev, 0))
+            r = 1;
+    } else {
+        if (libxl_device_pci_remove(ctx, domid, &pcidev, 0))
+            r = 1;
+    }
 
     libxl_device_pci_dispose(&pcidev);
     xlu_cfg_destroy(config);
+
+    return r;
 }
 
 int main_pcidetach(int argc, char **argv)
@@ -3853,13 +3859,17 @@ int main_pcidetach(int argc, char **argv)
     domid = find_domain(argv[optind]);
     bdf = argv[optind + 1];
 
-    pcidetach(domid, bdf, force);
-    return 0;
+    if (pcidetach(domid, bdf, force))
+        return EXIT_FAILURE;
+
+    return EXIT_SUCCESS;
 }
-static void pciattach(uint32_t domid, const char *bdf, const char *vs)
+
+static int pciattach(uint32_t domid, const char *bdf, const char *vs)
 {
     libxl_device_pci pcidev;
     XLU_Config *config;
+    int r = 0;
 
     libxl_device_pci_init(&pcidev);
 
@@ -3870,10 +3880,14 @@ static void pciattach(uint32_t domid, const char *bdf, const char *vs)
         fprintf(stderr, "pci-attach: malformed BDF specification \"%s\"\n", bdf);
         exit(2);
     }
-    libxl_device_pci_add(ctx, domid, &pcidev, 0);
+
+    if (libxl_device_pci_add(ctx, domid, &pcidev, 0))
+        r = 1;
 
     libxl_device_pci_dispose(&pcidev);
     xlu_cfg_destroy(config);
+
+    return r;
 }
 
 int main_pciattach(int argc, char **argv)
@@ -3892,8 +3906,10 @@ int main_pciattach(int argc, char **argv)
     if (optind + 1 < argc)
         vs = argv[optind + 2];
 
-    pciattach(domid, bdf, vs);
-    return 0;
+    if (pciattach(domid, bdf, vs))
+        return EXIT_FAILURE;
+
+    return EXIT_SUCCESS;
 }
 
 static void pciassignable_list(void)
@@ -3925,10 +3941,11 @@ int main_pciassignable_list(int argc, char **argv)
     return 0;
 }
 
-static void pciassignable_add(const char *bdf, int rebind)
+static int pciassignable_add(const char *bdf, int rebind)
 {
     libxl_device_pci pcidev;
     XLU_Config *config;
+    int r = 0;
 
     libxl_device_pci_init(&pcidev);
 
@@ -3939,10 +3956,14 @@ static void pciassignable_add(const char *bdf, int rebind)
         fprintf(stderr, "pci-assignable-add: malformed BDF specification \"%s\"\n", bdf);
         exit(2);
     }
-    libxl_device_pci_assignable_add(ctx, &pcidev, rebind);
+
+    if (libxl_device_pci_assignable_add(ctx, &pcidev, rebind))
+        r = 1;
 
     libxl_device_pci_dispose(&pcidev);
     xlu_cfg_destroy(config);
+
+    return r;
 }
 
 int main_pciassignable_add(int argc, char **argv)
@@ -3956,14 +3977,17 @@ int main_pciassignable_add(int argc, char **argv)
 
     bdf = argv[optind];
 
-    pciassignable_add(bdf, 1);
-    return 0;
+    if (pciassignable_add(bdf, 1))
+        return EXIT_FAILURE;
+
+    return EXIT_SUCCESS;
 }
 
-static void pciassignable_remove(const char *bdf, int rebind)
+static int pciassignable_remove(const char *bdf, int rebind)
 {
     libxl_device_pci pcidev;
     XLU_Config *config;
+    int r = 0;
 
     libxl_device_pci_init(&pcidev);
 
@@ -3974,10 +3998,14 @@ static void pciassignable_remove(const char *bdf, int rebind)
         fprintf(stderr, "pci-assignable-remove: malformed BDF specification \"%s\"\n", bdf);
         exit(2);
     }
-    libxl_device_pci_assignable_remove(ctx, &pcidev, rebind);
+
+    if (libxl_device_pci_assignable_remove(ctx, &pcidev, rebind))
+        r = 1;
 
     libxl_device_pci_dispose(&pcidev);
     xlu_cfg_destroy(config);
+
+    return r;
 }
 
 int main_pciassignable_remove(int argc, char **argv)
@@ -3994,8 +4022,10 @@ int main_pciassignable_remove(int argc, char **argv)
 
     bdf = argv[optind];
 
-    pciassignable_remove(bdf, rebind);
-    return 0;
+    if (pciassignable_remove(bdf, rebind))
+        return EXIT_FAILURE;
+
+    return EXIT_SUCCESS;
 }
 
 static void pause_domain(uint32_t domid)
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 4/7] xl_cmdimpl - Add return codes for pci-detach, pci-attach, pci-asssignable-add, and pci-assignable-remove.

[Wei Liu]

On Wed, Apr 20, 2016 at 10:04:01AM +0200, Paulina Szubarczyk wrote:
> Returning error codes makes it easier for shell scripts to tell if a
> command has failed or succeeded.
> 
> Signed-off-by: George Dunlap <George.Dunlap@eu.citrix.com>
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v3 5/7] xl: improve main_tmem_* return codes

[Paulina Szubarczyk]

Functions libxl_tmem_freeze(), libxl_tmem_thaw(), libxl_tmem_set() and
libxl_tmem_shared_auth() located in libxl.c file return
ERROR_FAIL/ERROR_INVAL or internal error codes from libxc library
improve main_tmem_* return codes by returning EXIT_{SUCCESS/FAILURE}
accordingly to return codes of those functions.

Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 51 ++++++++++++++++++++++++++++--------------------
 1 file changed, 30 insertions(+), 21 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 6bef201..d646927 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7817,11 +7817,11 @@ int main_tmem_list(int argc, char **argv)
 
     buf = libxl_tmem_list(ctx, domid, use_long);
     if (buf == NULL)
-        return -1;
+        return EXIT_FAILURE;
 
     printf("%s\n", buf);
     free(buf);
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 int main_tmem_freeze(int argc, char **argv)
@@ -7841,7 +7841,7 @@ int main_tmem_freeze(int argc, char **argv)
     if (!dom && all == 0) {
         fprintf(stderr, "You must specify -a or a domain id.\n\n");
         help("tmem-freeze");
-        return 1;
+        return EXIT_FAILURE;
     }
 
     if (all)
@@ -7849,8 +7849,10 @@ int main_tmem_freeze(int argc, char **argv)
     else
         domid = find_domain(dom);
 
-    libxl_tmem_freeze(ctx, domid);
-    return 0;
+    if (libxl_tmem_freeze(ctx, domid) < 0)
+        return EXIT_FAILURE;
+
+    return EXIT_SUCCESS;
 }
 
 int main_tmem_thaw(int argc, char **argv)
@@ -7870,7 +7872,7 @@ int main_tmem_thaw(int argc, char **argv)
     if (!dom && all == 0) {
         fprintf(stderr, "You must specify -a or a domain id.\n\n");
         help("tmem-thaw");
-        return 1;
+        return EXIT_FAILURE;
     }
 
     if (all)
@@ -7878,8 +7880,10 @@ int main_tmem_thaw(int argc, char **argv)
     else
         domid = find_domain(dom);
 
-    libxl_tmem_thaw(ctx, domid);
-    return 0;
+    if (libxl_tmem_thaw(ctx, domid) < 0)
+        return EXIT_FAILURE;
+
+    return EXIT_SUCCESS;
 }
 
 int main_tmem_set(int argc, char **argv)
@@ -7890,6 +7894,7 @@ int main_tmem_set(int argc, char **argv)
     int opt_w = 0, opt_c = 0, opt_p = 0;
     int all = 0;
     int opt;
+    int rc = 0;
 
     SWITCH_FOREACH_OPT(opt, "aw:c:p:", NULL, "tmem-set", 0) {
     case 'a':
@@ -7913,7 +7918,7 @@ int main_tmem_set(int argc, char **argv)
     if (!dom && all == 0) {
         fprintf(stderr, "You must specify -a or a domain id.\n\n");
         help("tmem-set");
-        return 1;
+        return EXIT_FAILURE;
     }
 
     if (all)
@@ -7924,17 +7929,20 @@ int main_tmem_set(int argc, char **argv)
     if (!opt_w && !opt_c && !opt_p) {
         fprintf(stderr, "No set value specified.\n\n");
         help("tmem-set");
-        return 1;
+        return EXIT_FAILURE;
     }
 
     if (opt_w)
-        libxl_tmem_set(ctx, domid, "weight", weight);
+        rc = libxl_tmem_set(ctx, domid, "weight", weight);
     if (opt_c)
-        libxl_tmem_set(ctx, domid, "cap", cap);
+        rc = libxl_tmem_set(ctx, domid, "cap", cap);
     if (opt_p)
-        libxl_tmem_set(ctx, domid, "compress", compress);
+        rc = libxl_tmem_set(ctx, domid, "compress", compress);
 
-    return 0;
+    if (rc < 0)
+        return EXIT_FAILURE;
+
+    return EXIT_SUCCESS;
 }
 
 int main_tmem_shared_auth(int argc, char **argv)
@@ -7964,7 +7972,7 @@ int main_tmem_shared_auth(int argc, char **argv)
     if (!dom && all == 0) {
         fprintf(stderr, "You must specify -a or a domain id.\n\n");
         help("tmem-shared-auth");
-        return 1;
+        return EXIT_FAILURE;
     }
 
     if (all)
@@ -7975,18 +7983,19 @@ int main_tmem_shared_auth(int argc, char **argv)
     if (uuid == NULL || autharg == NULL) {
         fprintf(stderr, "No uuid or auth specified.\n\n");
         help("tmem-shared-auth");
-        return 1;
+        return EXIT_FAILURE;
     }
 
     auth = strtol(autharg, &endptr, 10);
     if (*endptr != '\0') {
         fprintf(stderr, "Invalid auth, valid auth are <0|1>.\n\n");
-        return 1;
+        return EXIT_FAILURE;
     }
 
-    libxl_tmem_shared_auth(ctx, domid, uuid, auth);
+    if (libxl_tmem_shared_auth(ctx, domid, uuid, auth) < 0)
+        return EXIT_FAILURE;
 
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 int main_tmem_freeable(int argc, char **argv)
@@ -8000,10 +8009,10 @@ int main_tmem_freeable(int argc, char **argv)
 
     mb = libxl_tmem_freeable(ctx);
     if (mb == -1)
-        return -1;
+        return EXIT_FAILURE;
 
     printf("%d\n", mb);
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 int main_cpupoolcreate(int argc, char **argv)
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 5/7] xl: improve main_tmem_* return codes

[George Dunlap]

On 20/04/16 09:04, Paulina Szubarczyk wrote:
> Functions libxl_tmem_freeze(), libxl_tmem_thaw(), libxl_tmem_set() and
> libxl_tmem_shared_auth() located in libxl.c file return
> ERROR_FAIL/ERROR_INVAL or internal error codes from libxc library
> improve main_tmem_* return codes by returning EXIT_{SUCCESS/FAILURE}
> accordingly to return codes of those functions.
> 
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Paulina,

These patches are all basically independent, right?

If you move as many of the patches which have suitable Acked /
Reviewed-by's to the front of the series on the next iteration, then a
committer can check in the ones which are ready to be checked in
immediately, and subsequent series (if any) can be much shorter.

Although at this point it's more complicated because they now also need
a release ack to be checked in. :-)

 -George

> ---
>  tools/libxl/xl_cmdimpl.c | 51 ++++++++++++++++++++++++++++--------------------
>  1 file changed, 30 insertions(+), 21 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 6bef201..d646927 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -7817,11 +7817,11 @@ int main_tmem_list(int argc, char **argv)
>  
>      buf = libxl_tmem_list(ctx, domid, use_long);
>      if (buf == NULL)
> -        return -1;
> +        return EXIT_FAILURE;
>  
>      printf("%s\n", buf);
>      free(buf);
> -    return 0;
> +    return EXIT_SUCCESS;
>  }
>  
>  int main_tmem_freeze(int argc, char **argv)
> @@ -7841,7 +7841,7 @@ int main_tmem_freeze(int argc, char **argv)
>      if (!dom && all == 0) {
>          fprintf(stderr, "You must specify -a or a domain id.\n\n");
>          help("tmem-freeze");
> -        return 1;
> +        return EXIT_FAILURE;
>      }
>  
>      if (all)
> @@ -7849,8 +7849,10 @@ int main_tmem_freeze(int argc, char **argv)
>      else
>          domid = find_domain(dom);
>  
> -    libxl_tmem_freeze(ctx, domid);
> -    return 0;
> +    if (libxl_tmem_freeze(ctx, domid) < 0)
> +        return EXIT_FAILURE;
> +
> +    return EXIT_SUCCESS;
>  }
>  
>  int main_tmem_thaw(int argc, char **argv)
> @@ -7870,7 +7872,7 @@ int main_tmem_thaw(int argc, char **argv)
>      if (!dom && all == 0) {
>          fprintf(stderr, "You must specify -a or a domain id.\n\n");
>          help("tmem-thaw");
> -        return 1;
> +        return EXIT_FAILURE;
>      }
>  
>      if (all)
> @@ -7878,8 +7880,10 @@ int main_tmem_thaw(int argc, char **argv)
>      else
>          domid = find_domain(dom);
>  
> -    libxl_tmem_thaw(ctx, domid);
> -    return 0;
> +    if (libxl_tmem_thaw(ctx, domid) < 0)
> +        return EXIT_FAILURE;
> +
> +    return EXIT_SUCCESS;
>  }
>  
>  int main_tmem_set(int argc, char **argv)
> @@ -7890,6 +7894,7 @@ int main_tmem_set(int argc, char **argv)
>      int opt_w = 0, opt_c = 0, opt_p = 0;
>      int all = 0;
>      int opt;
> +    int rc = 0;
>  
>      SWITCH_FOREACH_OPT(opt, "aw:c:p:", NULL, "tmem-set", 0) {
>      case 'a':
> @@ -7913,7 +7918,7 @@ int main_tmem_set(int argc, char **argv)
>      if (!dom && all == 0) {
>          fprintf(stderr, "You must specify -a or a domain id.\n\n");
>          help("tmem-set");
> -        return 1;
> +        return EXIT_FAILURE;
>      }
>  
>      if (all)
> @@ -7924,17 +7929,20 @@ int main_tmem_set(int argc, char **argv)
>      if (!opt_w && !opt_c && !opt_p) {
>          fprintf(stderr, "No set value specified.\n\n");
>          help("tmem-set");
> -        return 1;
> +        return EXIT_FAILURE;
>      }
>  
>      if (opt_w)
> -        libxl_tmem_set(ctx, domid, "weight", weight);
> +        rc = libxl_tmem_set(ctx, domid, "weight", weight);
>      if (opt_c)
> -        libxl_tmem_set(ctx, domid, "cap", cap);
> +        rc = libxl_tmem_set(ctx, domid, "cap", cap);
>      if (opt_p)
> -        libxl_tmem_set(ctx, domid, "compress", compress);
> +        rc = libxl_tmem_set(ctx, domid, "compress", compress);
>  
> -    return 0;
> +    if (rc < 0)
> +        return EXIT_FAILURE;
> +
> +    return EXIT_SUCCESS;
>  }
>  
>  int main_tmem_shared_auth(int argc, char **argv)
> @@ -7964,7 +7972,7 @@ int main_tmem_shared_auth(int argc, char **argv)
>      if (!dom && all == 0) {
>          fprintf(stderr, "You must specify -a or a domain id.\n\n");
>          help("tmem-shared-auth");
> -        return 1;
> +        return EXIT_FAILURE;
>      }
>  
>      if (all)
> @@ -7975,18 +7983,19 @@ int main_tmem_shared_auth(int argc, char **argv)
>      if (uuid == NULL || autharg == NULL) {
>          fprintf(stderr, "No uuid or auth specified.\n\n");
>          help("tmem-shared-auth");
> -        return 1;
> +        return EXIT_FAILURE;
>      }
>  
>      auth = strtol(autharg, &endptr, 10);
>      if (*endptr != '\0') {
>          fprintf(stderr, "Invalid auth, valid auth are <0|1>.\n\n");
> -        return 1;
> +        return EXIT_FAILURE;
>      }
>  
> -    libxl_tmem_shared_auth(ctx, domid, uuid, auth);
> +    if (libxl_tmem_shared_auth(ctx, domid, uuid, auth) < 0)
> +        return EXIT_FAILURE;
>  
> -    return 0;
> +    return EXIT_SUCCESS;
>  }
>  
>  int main_tmem_freeable(int argc, char **argv)
> @@ -8000,10 +8009,10 @@ int main_tmem_freeable(int argc, char **argv)
>  
>      mb = libxl_tmem_freeable(ctx);
>      if (mb == -1)
> -        return -1;
> +        return EXIT_FAILURE;
>  
>      printf("%d\n", mb);
> -    return 0;
> +    return EXIT_SUCCESS;
>  }
>  
>  int main_cpupoolcreate(int argc, char **argv)
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 5/7] xl: improve main_tmem_* return codes

[Wei Liu]

On Wed, Apr 20, 2016 at 10:04:02AM +0200, Paulina Szubarczyk wrote:
> Functions libxl_tmem_freeze(), libxl_tmem_thaw(), libxl_tmem_set() and
> libxl_tmem_shared_auth() located in libxl.c file return
> ERROR_FAIL/ERROR_INVAL or internal error codes from libxc library
> improve main_tmem_* return codes by returning EXIT_{SUCCESS/FAILURE}
> accordingly to return codes of those functions.
> 
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v3 6/7] libxl: Fix libxl_set_memory_target return value

[Paulina Szubarczyk]

libxl_set_memory_target seems to have the following return values:

* 1 on failure, if the failure happens because of a xenstore error *or*
* invalid target

* -1 if the setmaxmem hypercall

* -errno if the set_pod_target hypercall target fails

* 0 on success

Make it consistently return ERROR_FAIL on failure, unless the
parameters were invalid, in which case return ERROR_INVAL.

In accordance with CODING_SYTLE:

1. Leave rc uninitialized, and set when an error is detected

2. Use 'r' for return values to functions whose return values are a
different error space (like xc_domain_setmaxmem and
xc_domain_set_pod_target)

3. Use 'lrc' for return values to local functions libxl__*
where a failure means retry, rather than fail the whole function
(libxl__fill_dom0_memory_info), to reduce the risk of that.

Signed-off-by: George Dunlap <George.Dunlap@eu.citrix.com>
Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
---
 tools/libxl/libxl.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index d232473..9130f74 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4839,7 +4839,7 @@ int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid,
         int32_t target_memkb, int relative, int enforce)
 {
     GC_INIT(ctx);
-    int rc = 1, abort_transaction = 0;
+    int rc, r, lrc, abort_transaction = 0;
     uint64_t memorykb;
     uint32_t videoram = 0;
     uint32_t current_target_memkb = 0, new_target_memkb = 0;
@@ -4867,15 +4867,15 @@ retry_transaction:
     if (!target && !domid) {
         if (!xs_transaction_end(ctx->xsh, t, 1))
             goto out_no_transaction;
-        rc = libxl__fill_dom0_memory_info(gc, &current_target_memkb,
+        lrc = libxl__fill_dom0_memory_info(gc, &current_target_memkb,
                                           &current_max_memkb);
-        if (rc < 0)
-            goto out_no_transaction;
+        if (lrc < 0) { rc = ERROR_FAIL; goto out_no_transaction; }
         goto retry_transaction;
     } else if (!target) {
         LOGE(ERROR, "cannot get target memory info from %s/memory/target",
              dompath);
         abort_transaction = 1;
+        rc = ERROR_FAIL;
         goto out;
     } else {
         current_target_memkb = strtoul(target, &endptr, 10);
@@ -4883,6 +4883,7 @@ retry_transaction:
             LOGE(ERROR, "invalid memory target %s from %s/memory/target\n",
                  target, dompath);
             abort_transaction = 1;
+            rc = ERROR_FAIL;
             goto out;
         }
     }
@@ -4891,6 +4892,7 @@ retry_transaction:
         LOGE(ERROR, "cannot get memory info from %s/memory/static-max",
              dompath);
         abort_transaction = 1;
+        rc = ERROR_FAIL;
         goto out;
     }
     memorykb = strtoul(memmax, &endptr, 10);
@@ -4898,6 +4900,7 @@ retry_transaction:
         LOGE(ERROR, "invalid max memory %s from %s/memory/static-max\n",
              memmax, dompath);
         abort_transaction = 1;
+        rc = ERROR_FAIL;
         goto out;
     }
 
@@ -4917,6 +4920,7 @@ retry_transaction:
             "memory_dynamic_max must be less than or equal to"
             " memory_static_max\n");
         abort_transaction = 1;
+        rc = ERROR_INVAL;
         goto out;
     }
 
@@ -4924,33 +4928,36 @@ retry_transaction:
         LOG(ERROR, "new target %d for dom0 is below the minimum threshold",
             new_target_memkb);
         abort_transaction = 1;
+        rc = ERROR_INVAL;
         goto out;
     }
 
     if (enforce) {
         memorykb = new_target_memkb + videoram;
-        rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb +
+        r = xc_domain_setmaxmem(ctx->xch, domid, memorykb +
                 LIBXL_MAXMEM_CONSTANT);
-        if (rc != 0) {
+        if (r != 0) {
             LOGE(ERROR,
                  "xc_domain_setmaxmem domid=%u memkb=%"PRIu64" failed ""rc=%d\n",
                  domid,
                  memorykb + LIBXL_MAXMEM_CONSTANT,
-                 rc);
+                 r);
             abort_transaction = 1;
+            rc = ERROR_FAIL;
             goto out;
         }
     }
 
-    rc = xc_domain_set_pod_target(ctx->xch, domid,
+    r = xc_domain_set_pod_target(ctx->xch, domid,
             (new_target_memkb + LIBXL_MAXMEM_CONSTANT) / 4, NULL, NULL, NULL);
-    if (rc != 0) {
+    if (r != 0) {
         LOGE(ERROR,
              "xc_domain_set_pod_target domid=%d, memkb=%d ""failed rc=%d\n",
              domid,
              new_target_memkb / 4,
-             rc);
+             r);
         abort_transaction = 1;
+        rc = ERROR_FAIL;
         goto out;
     }
 
@@ -4964,6 +4971,7 @@ retry_transaction:
                      "%"PRIu32, new_target_memkb / 1024);
     libxl_dominfo_dispose(&ptr);
 
+    rc = 0;
 out:
     if (!xs_transaction_end(ctx->xsh, t, abort_transaction)
         && !abort_transaction)
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 6/7] libxl: Fix libxl_set_memory_target return value

[Olaf Hering]

On Wed, Apr 20, Paulina Szubarczyk wrote:

> libxl_set_memory_target seems to have the following return values:
> 
> * 1 on failure, if the failure happens because of a xenstore error *or*
> * invalid target
> 
> * -1 if the setmaxmem hypercall
> 
> * -errno if the set_pod_target hypercall target fails
> 
> * 0 on success
> 
> Make it consistently return ERROR_FAIL on failure, unless the
> parameters were invalid, in which case return ERROR_INVAL.
> 
> In accordance with CODING_SYTLE:
> 
> 1. Leave rc uninitialized, and set when an error is detected
> 
> 2. Use 'r' for return values to functions whose return values are a
> different error space (like xc_domain_setmaxmem and
> xc_domain_set_pod_target)
> 
> 3. Use 'lrc' for return values to local functions libxl__*
> where a failure means retry, rather than fail the whole function
> (libxl__fill_dom0_memory_info), to reduce the risk of that.
> 
> Signed-off-by: George Dunlap <George.Dunlap@eu.citrix.com>
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>

Reviewed-by: Olaf Hering <olaf@aepfle.de>

Olaf

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 6/7] libxl: Fix libxl_set_memory_target return value

[Wei Liu]

On Wed, Apr 20, 2016 at 10:04:03AM +0200, Paulina Szubarczyk wrote:
> libxl_set_memory_target seems to have the following return values:
> 
> * 1 on failure, if the failure happens because of a xenstore error *or*
> * invalid target
> 
> * -1 if the setmaxmem hypercall

If the setmaxmem hypercall fails?

> 
> * -errno if the set_pod_target hypercall target fails
> 
> * 0 on success
> 
> Make it consistently return ERROR_FAIL on failure, unless the
> parameters were invalid, in which case return ERROR_INVAL.
> 

All in all the error code handling is not very sane in this function, so
I'm fine with fixing it with something better.

> In accordance with CODING_SYTLE:
> 
> 1. Leave rc uninitialized, and set when an error is detected
> 
> 2. Use 'r' for return values to functions whose return values are a
> different error space (like xc_domain_setmaxmem and
> xc_domain_set_pod_target)
> 
> 3. Use 'lrc' for return values to local functions libxl__*
> where a failure means retry, rather than fail the whole function
> (libxl__fill_dom0_memory_info), to reduce the risk of that.
> 
> Signed-off-by: George Dunlap <George.Dunlap@eu.citrix.com>
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>

The code looks good to me.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 6/7] libxl: Fix libxl_set_memory_target return value

[Paulina Szubarczyk]

On Wed, 2016-04-27 at 15:29 +0100, Wei Liu wrote:
> On Wed, Apr 20, 2016 at 10:04:03AM +0200, Paulina Szubarczyk wrote:
> > libxl_set_memory_target seems to have the following return values:
> > 
> > * 1 on failure, if the failure happens because of a xenstore error *or*
> > * invalid target
> > 
> > * -1 if the setmaxmem hypercall
> 
> If the setmaxmem hypercall fails?
The setmaxmem hypercall returns -1 and sets errno on error and in fact
is the same behavior for set_pod_target. I am going to corrected the log
to:
"
 '1' : on failure, if the failure happens because of a xenstore error
       *or* invalid target
 '-1': on error, the setmaxmem and set_pod_target hypercalls
       return -1 and set errno appropriately.
"
> 
> > 
> > * -errno if the set_pod_target hypercall target fails
> > 
> > * 0 on success
> > 
> > Make it consistently return ERROR_FAIL on failure, unless the
> > parameters were invalid, in which case return ERROR_INVAL.
> > 
> 
> All in all the error code handling is not very sane in this function, so
> I'm fine with fixing it with something better.
> 
> > In accordance with CODING_SYTLE:
> > 
> > 1. Leave rc uninitialized, and set when an error is detected
> > 
> > 2. Use 'r' for return values to functions whose return values are a
> > different error space (like xc_domain_setmaxmem and
> > xc_domain_set_pod_target)
> > 
> > 3. Use 'lrc' for return values to local functions libxl__*
> > where a failure means retry, rather than fail the whole function
> > (libxl__fill_dom0_memory_info), to reduce the risk of that.
> > 
> > Signed-off-by: George Dunlap <George.Dunlap@eu.citrix.com>
> > Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
> 
> The code looks good to me.
Could you help me understand if the comments like "looks good to me"
should be marked in any way in resending packets?
 
> Wei.

Paulina


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 6/7] libxl: Fix libxl_set_memory_target return value

[George Dunlap]

On Mon, May 9, 2016 at 9:21 AM, Paulina Szubarczyk
<paulinaszubarczyk@gmail.com> wrote:
> On Wed, 2016-04-27 at 15:29 +0100, Wei Liu wrote:
>> On Wed, Apr 20, 2016 at 10:04:03AM +0200, Paulina Szubarczyk wrote:
>> > libxl_set_memory_target seems to have the following return values:
>> >
>> > * 1 on failure, if the failure happens because of a xenstore error *or*
>> > * invalid target
>> >
>> > * -1 if the setmaxmem hypercall
>>
>> If the setmaxmem hypercall fails?
> The setmaxmem hypercall returns -1 and sets errno on error and in fact
> is the same behavior for set_pod_target. I am going to corrected the log
> to:
> "
>  '1' : on failure, if the failure happens because of a xenstore error
>        *or* invalid target
>  '-1': on error, the setmaxmem and set_pod_target hypercalls
>        return -1 and set errno appropriately.
> "
>>
>> >
>> > * -errno if the set_pod_target hypercall target fails
>> >
>> > * 0 on success
>> >
>> > Make it consistently return ERROR_FAIL on failure, unless the
>> > parameters were invalid, in which case return ERROR_INVAL.
>> >
>>
>> All in all the error code handling is not very sane in this function, so
>> I'm fine with fixing it with something better.
>>
>> > In accordance with CODING_SYTLE:
>> >
>> > 1. Leave rc uninitialized, and set when an error is detected
>> >
>> > 2. Use 'r' for return values to functions whose return values are a
>> > different error space (like xc_domain_setmaxmem and
>> > xc_domain_set_pod_target)
>> >
>> > 3. Use 'lrc' for return values to local functions libxl__*
>> > where a failure means retry, rather than fail the whole function
>> > (libxl__fill_dom0_memory_info), to reduce the risk of that.
>> >
>> > Signed-off-by: George Dunlap <George.Dunlap@eu.citrix.com>
>> > Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
>>
>> The code looks good to me.
> Could you help me understand if the comments like "looks good to me"
> should be marked in any way in resending packets?

No, it's just informational to you the sender.  It generally means, "I
would have given an Acked-by (or a Reviewed-by) if it weren't for the
other issues that I've raised."

Without that, as a submitter, you're a bit in the dark -- in this
case, you know that the reviewer has found some faults in the
changelog; but you don't know if they've even looked at the code, and
might find some other faults after you re-send it.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 6/7] libxl: Fix libxl_set_memory_target return value

[Wei Liu]

On Mon, May 09, 2016 at 10:21:09AM +0200, Paulina Szubarczyk wrote:
> On Wed, 2016-04-27 at 15:29 +0100, Wei Liu wrote:
> > On Wed, Apr 20, 2016 at 10:04:03AM +0200, Paulina Szubarczyk wrote:
> > > libxl_set_memory_target seems to have the following return values:
> > > 
> > > * 1 on failure, if the failure happens because of a xenstore error *or*
> > > * invalid target
> > > 
> > > * -1 if the setmaxmem hypercall
> > 
> > If the setmaxmem hypercall fails?

I said that because your original sentence looked incomplete.

> The setmaxmem hypercall returns -1 and sets errno on error and in fact
> is the same behavior for set_pod_target. I am going to corrected the log
> to:
> "
>  '1' : on failure, if the failure happens because of a xenstore error
>        *or* invalid target
>  '-1': on error, the setmaxmem and set_pod_target hypercalls
>        return -1 and set errno appropriately.
> "

This looks ok to me.

> > 
> > > 
> > > * -errno if the set_pod_target hypercall target fails
> > > 
> > > * 0 on success
> > > 
> > > Make it consistently return ERROR_FAIL on failure, unless the
> > > parameters were invalid, in which case return ERROR_INVAL.
> > > 
> > 
> > All in all the error code handling is not very sane in this function, so
> > I'm fine with fixing it with something better.
> > 
> > > In accordance with CODING_SYTLE:
> > > 
> > > 1. Leave rc uninitialized, and set when an error is detected
> > > 
> > > 2. Use 'r' for return values to functions whose return values are a
> > > different error space (like xc_domain_setmaxmem and
> > > xc_domain_set_pod_target)
> > > 
> > > 3. Use 'lrc' for return values to local functions libxl__*
> > > where a failure means retry, rather than fail the whole function
> > > (libxl__fill_dom0_memory_info), to reduce the risk of that.
> > > 
> > > Signed-off-by: George Dunlap <George.Dunlap@eu.citrix.com>
> > > Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
> > 
> > The code looks good to me.
> Could you help me understand if the comments like "looks good to me"
> should be marked in any way in resending packets?
>  

What George said.

In this particular instance, I've looked at the code, but the commit
message was not clear enough. I expect you to address my question by
either replying to my mail saying why the original commit message is
correct, or resend the patch with new correct commit message.

BTW, your next patch also had some comments that you need to address.

Wei.

> > Wei.
> 
> Paulina
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v3 7/7] libxl: libxl_tmem functions improving coding style

[Paulina Szubarczyk]

In accordance with CODING_SYTLE:
 - Use 'r' for return values to functions whose return values are a
   different error space (like xc_tmem_control, xc_tmem_auth)

libxc functions are supposed to, on failure, set errno and alwas return -1 
which is the value stored in 'r', therfore use LOG() instead LOGEV() 
with the 'r' value since the error code is always -1.

Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
---
Changed since v2:
 * corrected indentation
 * changed LOGEV()->LOG()
---
 tools/libxl/libxl.c | 89 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 50 insertions(+), 39 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 9130f74..02b6164 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -6597,14 +6597,14 @@ uint32_t libxl_vm_get_start_time(libxl_ctx *ctx, uint32_t domid)
 
 char *libxl_tmem_list(libxl_ctx *ctx, uint32_t domid, int use_long)
 {
-    int rc;
+    int r;
     char _buf[32768];
     GC_INIT(ctx);
 
-    rc = xc_tmem_control(ctx->xch, -1, XEN_SYSCTL_TMEM_OP_LIST, domid, 32768, use_long,
-                         _buf);
-    if (rc < 0) {
-        LOGEV(ERROR, rc, "Can not get tmem list");
+    r = xc_tmem_control(ctx->xch, -1, XEN_SYSCTL_TMEM_OP_LIST, domid, 32768,
+                        use_long, _buf);
+    if (r < 0) {
+        LOG(ERROR, "Can not get tmem list");
         GC_FREE;
         return NULL;
     }
@@ -6615,34 +6615,38 @@ char *libxl_tmem_list(libxl_ctx *ctx, uint32_t domid, int use_long)
 
 int libxl_tmem_freeze(libxl_ctx *ctx, uint32_t domid)
 {
-    int rc;
+    int r, rc;
     GC_INIT(ctx);
 
-    rc = xc_tmem_control(ctx->xch, -1, XEN_SYSCTL_TMEM_OP_FREEZE, domid, 0, 0,
-                         NULL);
-    if (rc < 0) {
-        LOGEV(ERROR, rc, "Can not freeze tmem pools");
-        GC_FREE;
-        return ERROR_FAIL;
+    r = xc_tmem_control(ctx->xch, -1, XEN_SYSCTL_TMEM_OP_FREEZE, domid, 0, 0,
+                        NULL);
+    if (r < 0) {
+        LOG(ERROR, "Can not freeze tmem pools");
+        rc = ERROR_FAIL;
+        goto out;
     }
 
+    rc = 0;
+out:
     GC_FREE;
     return rc;
 }
 
 int libxl_tmem_thaw(libxl_ctx *ctx, uint32_t domid)
 {
-    int rc;
+    int r, rc;
     GC_INIT(ctx);
 
-    rc = xc_tmem_control(ctx->xch, -1, XEN_SYSCTL_TMEM_OP_THAW, domid, 0, 0,
-                         NULL);
-    if (rc < 0) {
-        LOGEV(ERROR, rc, "Can not thaw tmem pools");
-        GC_FREE;
-        return ERROR_FAIL;
+    r = xc_tmem_control(ctx->xch, -1, XEN_SYSCTL_TMEM_OP_THAW, domid, 0, 0,
+                        NULL);
+    if (r < 0) {
+        LOG(ERROR, "Can not thaw tmem pools");
+        rc = ERROR_FAIL;
+        goto out;
     }
 
+    rc = 0;
+out:
     GC_FREE;
     return rc;
 }
@@ -6661,22 +6665,24 @@ static int32_t tmem_setop_from_string(char *set_name)
 
 int libxl_tmem_set(libxl_ctx *ctx, uint32_t domid, char* name, uint32_t set)
 {
-    int rc;
+    int r, rc;
     int32_t subop = tmem_setop_from_string(name);
     GC_INIT(ctx);
 
     if (subop == -1) {
         LOGEV(ERROR, -1, "Invalid set, valid sets are <weight|cap|compress>");
-        GC_FREE;
-        return ERROR_INVAL;
+        rc = ERROR_INVAL;
+        goto out;
     }
-    rc = xc_tmem_control(ctx->xch, -1, subop, domid, set, 0, NULL);
-    if (rc < 0) {
-        LOGEV(ERROR, rc, "Can not set tmem %s", name);
-        GC_FREE;
-        return ERROR_FAIL;
+    r = xc_tmem_control(ctx->xch, -1, subop, domid, set, 0, NULL);
+    if (r < 0) {
+        LOG(ERROR, "Can not set tmem %s", name);
+        rc = ERROR_FAIL;
+        goto out;
     }
 
+    rc = 0;
+out:
     GC_FREE;
     return rc;
 }
@@ -6684,32 +6690,37 @@ int libxl_tmem_set(libxl_ctx *ctx, uint32_t domid, char* name, uint32_t set)
 int libxl_tmem_shared_auth(libxl_ctx *ctx, uint32_t domid,
                            char* uuid, int auth)
 {
-    int rc;
+    int r, rc;
     GC_INIT(ctx);
 
-    rc = xc_tmem_auth(ctx->xch, domid, uuid, auth);
-    if (rc < 0) {
-        LOGEV(ERROR, rc, "Can not set tmem shared auth");
-        GC_FREE;
-        return ERROR_FAIL;
+    r = xc_tmem_auth(ctx->xch, domid, uuid, auth);
+    if (r < 0) {
+        LOG(ERROR, "Can not set tmem shared auth");
+        rc = ERROR_FAIL;
+        goto out;
     }
 
+    rc = 0;
+out:
     GC_FREE;
     return rc;
 }
 
 int libxl_tmem_freeable(libxl_ctx *ctx)
 {
-    int rc;
+    int r, rc;
     GC_INIT(ctx);
 
-    rc = xc_tmem_control(ctx->xch, -1, XEN_SYSCTL_TMEM_OP_QUERY_FREEABLE_MB, -1, 0, 0, 0);
-    if (rc < 0) {
-        LOGEV(ERROR, rc, "Can not get tmem freeable memory");
-        GC_FREE;
-        return ERROR_FAIL;
+    r = xc_tmem_control(ctx->xch, -1, XEN_SYSCTL_TMEM_OP_QUERY_FREEABLE_MB,
+                        -1, 0, 0, 0);
+    if (r < 0) {
+        LOG(ERROR, "Can not get tmem freeable memory");
+        rc = ERROR_FAIL;
+        goto out;
     }
 
+    rc = 0;
+out:
     GC_FREE;
     return rc;
 }
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 7/7] libxl: libxl_tmem functions improving coding style

[Olaf Hering]

On Wed, Apr 20, Paulina Szubarczyk wrote:

> In accordance with CODING_SYTLE:
>  - Use 'r' for return values to functions whose return values are a
>    different error space (like xc_tmem_control, xc_tmem_auth)
> 
> libxc functions are supposed to, on failure, set errno and alwas return -1 
> which is the value stored in 'r', therfore use LOG() instead LOGEV() 
> with the 'r' value since the error code is always -1.

Shouldnt in this case 'LOGEV' be replaced with 'LOGE' to get errno
printed? Plain 'LOG' does not seem to print errno.

Olaf

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 7/7] libxl: libxl_tmem functions improving coding style

[Wei Liu]

On Wed, Apr 20, 2016 at 11:04:06PM +0200, Olaf Hering wrote:
> On Wed, Apr 20, Paulina Szubarczyk wrote:
> 
> > In accordance with CODING_SYTLE:
> >  - Use 'r' for return values to functions whose return values are a
> >    different error space (like xc_tmem_control, xc_tmem_auth)
> > 
> > libxc functions are supposed to, on failure, set errno and alwas return -1 
> > which is the value stored in 'r', therfore use LOG() instead LOGEV() 
> > with the 'r' value since the error code is always -1.
> 
> Shouldnt in this case 'LOGEV' be replaced with 'LOGE' to get errno
> printed? Plain 'LOG' does not seem to print errno.
> 

Agreed.  We should use LOGEV here.

Wei.

> Olaf

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 7/7] libxl: libxl_tmem functions improving coding style

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1487 bytes --]

On Wed, 2016-04-27 at 15:31 +0100, Wei Liu wrote:
> On Wed, Apr 20, 2016 at 11:04:06PM +0200, Olaf Hering wrote:
> > 
> > On Wed, Apr 20, Paulina Szubarczyk wrote:
> > 
> > > 
> > > In accordance with CODING_SYTLE:
> > >  - Use 'r' for return values to functions whose return values are
> > > a
> > >    different error space (like xc_tmem_control, xc_tmem_auth)
> > > 
> > > libxc functions are supposed to, on failure, set errno and alwas
> > > return -1 
> > > which is the value stored in 'r', therfore use LOG() instead
> > > LOGEV() 
> > > with the 'r' value since the error code is always -1.
> > Shouldnt in this case 'LOGEV' be replaced with 'LOGE' to get errno
> > printed? Plain 'LOG' does not seem to print errno.
> > 
> Agreed.  We should use LOGEV here.
> 
You mean LOGE, don't you? :-)

I think it was me that suggested dropping LOGEV in favour of LOG, and I
agree that I should have suggested LOGE. Sorry for that.

TBF, I seem to find both cases in libxl source code (i.e., we use both
LOG and LOGE to log error after an xc_* call), but I agree that, since
libxc sets errno, LOGE is the better, and we should try to converge to
always using that one.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 7/7] libxl: libxl_tmem functions improving coding style

[Wei Liu]

On Wed, Apr 27, 2016 at 06:53:35PM +0200, Dario Faggioli wrote:
> On Wed, 2016-04-27 at 15:31 +0100, Wei Liu wrote:
> > On Wed, Apr 20, 2016 at 11:04:06PM +0200, Olaf Hering wrote:
> > > 
> > > On Wed, Apr 20, Paulina Szubarczyk wrote:
> > > 
> > > > 
> > > > In accordance with CODING_SYTLE:
> > > >  - Use 'r' for return values to functions whose return values are
> > > > a
> > > >    different error space (like xc_tmem_control, xc_tmem_auth)
> > > > 
> > > > libxc functions are supposed to, on failure, set errno and alwas
> > > > return -1 
> > > > which is the value stored in 'r', therfore use LOG() instead
> > > > LOGEV() 
> > > > with the 'r' value since the error code is always -1.
> > > Shouldnt in this case 'LOGEV' be replaced with 'LOGE' to get errno
> > > printed? Plain 'LOG' does not seem to print errno.
> > > 
> > Agreed.  We should use LOGEV here.
> > 
> You mean LOGE, don't you? :-)
> 

Yes, LOGE. Sorry.

> I think it was me that suggested dropping LOGEV in favour of LOG, and I
> agree that I should have suggested LOGE. Sorry for that.
> 
> TBF, I seem to find both cases in libxl source code (i.e., we use both
> LOG and LOGE to log error after an xc_* call), but I agree that, since
> libxc sets errno, LOGE is the better, and we should try to converge to
> always using that one.
> 

Yes.

Wei.

> Regards,
> Dario
> -- 
> <<This happens because I choose it to happen!>> (Raistlin Majere)
> -----------------------------------------------------------------
> Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 00/10] xl: improve coding style and return more failure on

[Wei Liu]

Hi Paulina

Sorry for the late reply. Now I finally have time to go over your
series.

All in all it is in very good shape. Feel free to ask questions if my
comment is not clear. You can resubmit this series when you get around
to fix the issues. Note that we're in code freeze now so this series
will need to wait until development window opens.

Thanks for your work!

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel