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

[PATCH v4 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();  

 Changed since v3:
 * The pcidev_struct_fill() function is now void
 * Corrected indentation
 * Dropped opendir(NULL) call in the libxl_device_pci_assignable_list() 
   function
 * Change in logging of libxc error 'LOG() -> LOGE();  

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

[PATCH v4 1/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>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
 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 e97599a..0e5826b 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 v4 1/7] xl_cmdimpl - Add return codes for pci-detach, pci-attach, pci-asssignable-add, and pci-assignable-remove.

[Wei Liu]

Hi Paulina

I was about to push this whole series, but I noticed the subject line
of this patch is very long.

May I make a suggestion that we update the title of this patch to

  xl: add return codes for various pci functions

?

If you agree I will do the modification and push it to staging.

Wei.

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

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

[Paulina Szubarczyk]

On Thu, 2016-06-09 at 15:44 +0100, Wei Liu wrote:
> Hi Paulina
> 
> I was about to push this whole series, but I noticed the subject line
> of this patch is very long.
> 
> May I make a suggestion that we update the title of this patch to
> 
>   xl: add return codes for various pci functions
> 
> ?
> 
> If you agree I will do the modification and push it to staging.

Hi Wei, 

yes, I do agree on the suggested title. Thank you.

Paulina



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

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

[Wei Liu]

On Thu, Jun 09, 2016 at 04:53:29PM +0200, Paulina Szubarczyk wrote:
> On Thu, 2016-06-09 at 15:44 +0100, Wei Liu wrote:
> > Hi Paulina
> > 
> > I was about to push this whole series, but I noticed the subject line
> > of this patch is very long.
> > 
> > May I make a suggestion that we update the title of this patch to
> > 
> >   xl: add return codes for various pci functions
> > 
> > ?
> > 
> > If you agree I will do the modification and push it to staging.
> 
> Hi Wei, 
> 
> yes, I do agree on the suggested title. Thank you.
> 

Thanks for your confirmation. Now your series has been pushed to
staging.

Wei.

> Paulina
> 
> 

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

[PATCH v4 2/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>
Acked-by: Wei Liu <wei.liu2@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 0e5826b..aedf613 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7821,11 +7821,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)
@@ -7845,7 +7845,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)
@@ -7853,8 +7853,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)
@@ -7874,7 +7876,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)
@@ -7882,8 +7884,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)
@@ -7894,6 +7898,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':
@@ -7917,7 +7922,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)
@@ -7928,17 +7933,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)
@@ -7968,7 +7976,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)
@@ -7979,18 +7987,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)
@@ -8004,10 +8013,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

[PATCH v4 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>
Reviewed-by: Olaf Hering <olaf@aepfle.de>
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 03ab644..e97599a 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 v4 3/7] xl_cmdimpl: improve return codes for cd-insert commands

[Wei Liu]

On Mon, May 09, 2016 at 01:30:54PM +0200, 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>
> Reviewed-by: Olaf Hering <olaf@aepfle.de>
> 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 v4 4/7] libxl: Fix libxl_set_memory_target return value

[Paulina Szubarczyk]

From: George Dunlap <George.Dunlap@eu.citrix.com>

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': on error, the setmaxmem and set_pod_target hypercalls
       return -1 and set errno appropriately.
 '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>
---
Changed since v3:
 - changed the changelog
 
 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 c39d745..7cb15bf 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 v4 4/7] libxl: Fix libxl_set_memory_target return value

[Wei Liu]

On Mon, May 09, 2016 at 01:30:55PM +0200, Paulina Szubarczyk wrote:
> From: George Dunlap <George.Dunlap@eu.citrix.com>
> 
> 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': on error, the setmaxmem and set_pod_target hypercalls
>        return -1 and set errno appropriately.
>  '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>

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

Thanks Olaf for reviewing btw.

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

[PATCH v4 5/7] libxl: improve return codes for some pci related functions

[Paulina Szubarczyk]

*libxl__device_from_pcidev(), pcidev_struct_fill() initialize
 the values of libxl_device and libxl_device_pci structs
 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>
---
Changed since v3:
 - pcidev_struct_fill() is now void
 - corrected indention
  
 tools/libxl/libxl_pci.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index ce8d763..4e2f56e 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -36,7 +36,7 @@ static unsigned int pcidev_encode_bdf(libxl_device_pci *pcidev)
     return value;
 }
 
-static int pcidev_struct_fill(libxl_device_pci *pcidev, unsigned int domain,
+static void pcidev_struct_fill(libxl_device_pci *pcidev, unsigned int domain,
                                unsigned int bus, unsigned int dev,
                                unsigned int func, unsigned int vdevfn)
 {
@@ -45,7 +45,6 @@ static int pcidev_struct_fill(libxl_device_pci *pcidev, unsigned int domain,
     pcidev->dev = dev;
     pcidev->func = func;
     pcidev->vdevfn = vdevfn;
-    return 0;
 }
 
 static void libxl_create_pci_backend_device(libxl__gc *gc, flexarray_t *back, int num, libxl_device_pci *pcidev)
@@ -64,9 +63,9 @@ 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,
-                                     libxl_device_pci *pcidev,
-                                     libxl__device *device)
+static void libxl__device_from_pcidev(libxl__gc *gc, uint32_t domid,
+                                      libxl_device_pci *pcidev,
+                                      libxl__device *device)
 {
     device->backend_devid = 0;
     device->backend_domid = 0;
@@ -74,8 +73,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 +81,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 +103,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 +1605,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 v4 5/7] libxl: improve return codes for some pci related functions

[Wei Liu]

On Mon, May 09, 2016 at 01:30:56PM +0200, Paulina Szubarczyk wrote:
> *libxl__device_from_pcidev(), pcidev_struct_fill() initialize
>  the values of libxl_device and libxl_device_pci structs
>  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>

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

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

[PATCH v4 6/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 v3:
 - When the opendir() returns NULL stored in 'dir' variable
   there is no need to call closedir(dir) in this path and out_closedir 
   may be dropped.
---
 tools/libxl/libxl_pci.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 4e2f56e..236bdd0 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -397,34 +397,33 @@ 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;
+        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;
@@ -435,7 +434,6 @@ libxl_device_pci *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num)
         (*num)++;
     }
 
-out_closedir:
     closedir(dir);
 out:
     GC_FREE;
-- 
1.9.1


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

Re: [PATCH v4 6/7] libxl: style cleanups in libxl_device_pci_assignable_list()

[Wei Liu]

On Mon, May 09, 2016 at 01:30:57PM +0200, Paulina Szubarczyk wrote:
> 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>

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

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

[PATCH v4 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 always
return -1  which is the value stored in 'r', therfore use LOGE()
instead LOGEV() with the 'r' value.

Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
---
Changed since v3:
 - changed LOG() to LOGE() 

 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 7cb15bf..3e5d97a 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -6620,14 +6620,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) {
+        LOGE(ERROR, "Can not get tmem list");
         GC_FREE;
         return NULL;
     }
@@ -6638,34 +6638,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) {
+        LOGE(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) {
+        LOGE(ERROR, "Can not thaw tmem pools");
+        rc = ERROR_FAIL;
+        goto out;
     }
 
+    rc = 0;
+out:
     GC_FREE;
     return rc;
 }
@@ -6684,22 +6688,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) {
+        LOGE(ERROR, "Can not set tmem %s", name);
+        rc = ERROR_FAIL;
+        goto out;
     }
 
+    rc = 0;
+out:
     GC_FREE;
     return rc;
 }
@@ -6707,32 +6713,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) {
+        LOGE(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) {
+        LOGE(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 v4 7/7] libxl: libxl_tmem functions improving coding style

[Wei Liu]

On Mon, May 09, 2016 at 01:30:58PM +0200, 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 always
> return -1  which is the value stored in 'r', therfore use LOGE()
> instead LOGEV() with the 'r' value.
> 
> 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

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

[Wei Liu]

Hi Paulina

I believe this series is now all acked.

We're however in code freeze at the moment -- only critical bug fixes
and patches very of very low risk can be committed. I will apply this
series when the tree reopens.

I have a small suggestion for you. You seemed to have shuffled the order
of the patches around (vs v3). It's better to avoid doing that in the
future unless necessary.

Wei.

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

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

[Paulina Szubarczyk]

On Mon, 2016-05-09 at 14:06 +0100, Wei Liu wrote:
> Hi Paulina
> 
> I believe this series is now all acked.
> 
> We're however in code freeze at the moment -- only critical bug fixes
> and patches very of very low risk can be committed. I will apply this
> series when the tree reopens.
Hi Wei, 
 
Thank you and all who helped me to submit the series for all the remarks
and looking at the patches. 

> 
> I have a small suggestion for you. You seemed to have shuffled the order
> of the patches around (vs v3). It's better to avoid doing that in the
> future unless necessary.
I am sorry for all the inconvenience I made concerning formatting/
submitting the patches. I have changed the order due to the suggestion
from George which I might wrongly understand (under v3 5/7). :)

Paulina

> Wei.



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

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

[Wei Liu]

On Mon, May 09, 2016 at 03:25:18PM +0200, Paulina Szubarczyk wrote:
> On Mon, 2016-05-09 at 14:06 +0100, Wei Liu wrote:
> > Hi Paulina
> > 
> > I believe this series is now all acked.
> > 
> > We're however in code freeze at the moment -- only critical bug fixes
> > and patches very of very low risk can be committed. I will apply this
> > series when the tree reopens.
> Hi Wei, 
>  
> Thank you and all who helped me to submit the series for all the remarks
> and looking at the patches. 
> 
> > 
> > I have a small suggestion for you. You seemed to have shuffled the order
> > of the patches around (vs v3). It's better to avoid doing that in the
> > future unless necessary.
> I am sorry for all the inconvenience I made concerning formatting/
> submitting the patches. I have changed the order due to the suggestion
> from George which I might wrongly understand (under v3 5/7). :)
> 

Ah, I see. Not a big deal really -- I thought you mistakenly shuffled
things around.  In that case it would be helpful to say so in the commit
message.

Wei.

> Paulina
> 
> > Wei.
> 
> 

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