[v4,5/5] xl: Return error codes for pci* commands

Commit Message

George Dunlap Dec. 18, 2015, 12:23 p.m. UTC

Add return codes for pci-detach, pci-attach, pci-asssignable-add, and
pci-assignable-remove.

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

NB this violates the CODING_STYLE preference for not initializing the
return-value variable at declaration; but in these cases, having a
"goto out" that jumped over nothing but an "rc = EXIT_SUCCESS" seemed
a bit pointless.

Signed-off-by: George Dunlap <[e-mail]>
---
v4:
 - Use EXIT_{SUCCESS,FAILURE} rather than magic constants.
 - Use 'r' rather than 'rc' for non-libxl error codes

CC: Ian Campbell <[e-mail]>
CC: Ian Jackson <[e-mail]>
CC: Wei Liu <[e-mail]>
---
 tools/libxl/xl_cmdimpl.c | 56 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 37 insertions(+), 19 deletions(-)

Comments

Dario Faggioli Dec. 21, 2015, 10:51 a.m. UTC | #1

On Fri, 2015-12-18 at 12:23 +0000, George Dunlap wrote:
> Add return codes for pci-detach, pci-attach, pci-asssignable-add, and
> pci-assignable-remove.
> 
> Returning error codes makes it easier for shell scripts to tell if a
> command has failed or succeeded.
> 
> NB this violates the CODING_STYLE preference for not initializing the
> return-value variable at declaration; but in these cases, having a
> "goto out" that jumped over nothing but an "rc = EXIT_SUCCESS" seemed
> a bit pointless.
>
You mean 'nothing but an "r = EXIT_SUCCESS"'... Or this changelog
violates the CODING_STYLE itself!  ;-P ;-P

> Signed-off-by: George Dunlap <[e-mail]>
>
Reviewed-by: Dario Faggioli <[e-mail]>

Dario

Ian Jackson Jan. 4, 2016, 5:40 p.m. UTC | #2

George Dunlap writes ("[PATCH v4 5/5] xl: Return error codes for pci* commands"):
> Add return codes for pci-detach, pci-attach, pci-asssignable-add, and
> pci-assignable-remove.
> 
> Returning error codes makes it easier for shell scripts to tell if a
> command has failed or succeeded.
> 
> NB this violates the CODING_STYLE preference for not initializing the
> return-value variable at declaration; but in these cases, having a
> "goto out" that jumped over nothing but an "rc = EXIT_SUCCESS" seemed
> a bit pointless.

(You mean `nothing but an "r = EXIT_SUCCESS"'.)

I'm afraid I am going to say that I would prefer the CODING_STYLE
approach.  Most relevantly, while it may seem pointless in this case,
it means that: (a) when another subroutine call is added to one of
these functions, it is not necessary to rework the error handling
again; and (b) everything is the same everywhere so there is less
scope for confusion.

Ian.

Patch

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 0c3756b..e0c962e 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3491,10 +3491,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 = EXIT_SUCCESS;
 
     libxl_device_pci_init(&pcidev);
 
@@ -3505,13 +3506,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 = EXIT_FAILURE;
+    } else {
+        if(libxl_device_pci_remove(ctx, domid, &pcidev, 0))
+            r = EXIT_FAILURE;
+    }
 
     libxl_device_pci_dispose(&pcidev);
     xlu_cfg_destroy(config);
+
+    return r;
 }
 
 int main_pcidetach(int argc, char **argv)
@@ -3530,13 +3536,14 @@  int main_pcidetach(int argc, char **argv)
     domid = find_domain(argv[optind]);
     bdf = argv[optind + 1];
 
-    pcidetach(domid, bdf, force);
-    return 0;
+    return pcidetach(domid, bdf, force);
 }
-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 = EXIT_SUCCESS;
 
     libxl_device_pci_init(&pcidev);
 
@@ -3547,10 +3554,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 = EXIT_FAILURE;
 
     libxl_device_pci_dispose(&pcidev);
     xlu_cfg_destroy(config);
+
+    return r;
 }
 
 int main_pciattach(int argc, char **argv)
@@ -3569,8 +3580,7 @@  int main_pciattach(int argc, char **argv)
     if (optind + 1 < argc)
         vs = argv[optind + 2];
 
-    pciattach(domid, bdf, vs);
-    return 0;
+    return pciattach(domid, bdf, vs);
 }
 
 static void pciassignable_list(void)
@@ -3602,10 +3612,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 = EXIT_SUCCESS;
 
     libxl_device_pci_init(&pcidev);
 
@@ -3616,10 +3627,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 = EXIT_FAILURE;
 
     libxl_device_pci_dispose(&pcidev);
     xlu_cfg_destroy(config);
+
+    return r;
 }
 
 int main_pciassignable_add(int argc, char **argv)
@@ -3633,14 +3648,14 @@  int main_pciassignable_add(int argc, char **argv)
 
     bdf = argv[optind];
 
-    pciassignable_add(bdf, 1);
-    return 0;
+    return pciassignable_add(bdf, 1);
 }
 
-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 = EXIT_SUCCESS;
 
     libxl_device_pci_init(&pcidev);
 
@@ -3651,10 +3666,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 = EXIT_FAILURE;
 
     libxl_device_pci_dispose(&pcidev);
     xlu_cfg_destroy(config);
+
+    return r;
 }
 
 int main_pciassignable_remove(int argc, char **argv)
@@ -3671,8 +3690,7 @@  int main_pciassignable_remove(int argc, char **argv)
 
     bdf = argv[optind];
 
-    pciassignable_remove(bdf, rebind);
-    return 0;
+    return pciassignable_remove(bdf, rebind);
 }
 
 static void pause_domain(uint32_t domid)