Hi Vu, Ack from me. Please correct the output in my second comment. You can correct it when you puch the code.
Thanks, Zoran ________________________________ From: Vu Minh Nguyen <[email protected]> Sent: Tuesday, November 14, 2017 7:16:29 AM To: Zoran Milinkovic Cc: [email protected] Subject: RE: [PATCH 1/1] clm: clmprint does not work as expected [#2651] Hi Zoran, See my responses inline. Regards, Vu > -----Original Message----- > From: Zoran Milinkovic [mailto:[email protected]] > Sent: Monday, November 13, 2017 9:35 PM > To: Vu Minh Nguyen <[email protected]> > Cc: [email protected]; Vu Minh Nguyen > <[email protected]> > Subject: RE: [PATCH 1/1] clm: clmprint does not work as expected [#2651] > > Hi Vu, > > Find my comments inline > > -----Original Message----- > From: Vu Minh Nguyen [mailto:[email protected]] > Sent: den 1 november 2017 12:29 > To: Zoran Milinkovic <[email protected]> > Cc: [email protected]; Vu Minh Nguyen > <[email protected]> > Subject: [PATCH 1/1] clm: clmprint does not work as expected [#2651] > > clmprint should exit with EXIT_FAILURE when querying non-member node. > --- > src/clm/tools/clm_print.c | 34 ++++++++++++++++++---------------- > 1 file changed, 18 insertions(+), 16 deletions(-) > > diff --git a/src/clm/tools/clm_print.c b/src/clm/tools/clm_print.c index > f44aee2..25b5e35 100644 > --- a/src/clm/tools/clm_print.c > +++ b/src/clm/tools/clm_print.c > @@ -454,12 +454,12 @@ int main(int argc, char *argv[]) > rc = saClmClusterNodeGet_4(clm_handle, node_id, > ((timeout == -1) ? (TIME_OUT) : timeout), > &clusterNode); > if (rc == SA_AIS_ERR_NOT_EXIST) { > - fprintf(stdout, "Node id 0x%x is not in cluster > membership\n", node_id); > - exit(EXIT_SUCCESS); > + fprintf(stderr, "Node id 0x%x is not in cluster > membership!\n", > +node_id); > } else if (rc == SA_AIS_ERR_UNAVAILABLE) { > - fprintf(stdout, "error - invoking clmprint from non- > member node\n"); > - exit(EXIT_FAILURE); > - } else if (rc != SA_AIS_OK) { > + fprintf(stderr, "invoking clmprint from non-member > node!\n"); > + } > > [Zoran] clm_print will print two error messages if rc is ERR_NOT_EXIST or > ERR_UNAVAILABLE. The first error message will be from the code above, and > the second error message will be from the IF statement below. > There is nothing wrong to put exit() in IF branches above. [Vu] Yes. It is my intention. First message is for non-developers user who don't care/know much about CLM APIs. Second one is for developers - show failed API and error code. Regarding exit(), to avoid duplicated codes, I moved `exit(EXIT_FAILURE)` to the below IF statement. Thanks, Vu > > + > + if (rc != SA_AIS_OK) { > fprintf(stderr, "error - clmprint:: > saClmClusterNodeGet_4 failed, rc = %d\n", rc); > exit(EXIT_FAILURE); > } > @@ -470,12 +470,12 @@ int main(int argc, char *argv[]) > case 'a': > rc = saClmClusterNodeGetAsync(clm_handle, INVOCATION_ID, > node_id); > if (rc == SA_AIS_ERR_NOT_EXIST) { > - fprintf(stdout, "Node id 0x%x is not in cluster > membership\n", node_id); > - exit(EXIT_SUCCESS); > + fprintf(stderr, "Node id 0x%x is not in cluster > membership!\n", > +node_id); > } else if (rc == SA_AIS_ERR_UNAVAILABLE) { > - fprintf(stdout, "error - invoking clmprint from non- > member node\n"); > - exit(EXIT_FAILURE); > - } else if (rc != SA_AIS_OK) { > + fprintf(stdout, "invoking clmprint from non-member > node!\n"); > + } > > [Zoran] Same as the first comment plus the fprintf() from the IF branch for rc > == SA_AIS_ERR_UNAVAILABLE should follow the output as other error > messages and print to stderr output. > > + > + if (rc != SA_AIS_OK) { > fprintf(stderr, "error - clmprint :: > saClmClusterNodeGetAsync failed, rc = %d\n", rc); > exit(EXIT_FAILURE); > } > @@ -487,9 +487,10 @@ int main(int argc, char *argv[]) > clusterNotificationBuffer.numberOfItems = > SIZE_NOTIFICATIONS; > rc = saClmClusterTrack_4(clm_handle, SA_TRACK_CURRENT, > &clusterNotificationBuffer); > if (rc == SA_AIS_ERR_UNAVAILABLE) { > - fprintf(stdout, "error - invoking clmprint from non- > member node\n"); > - exit(EXIT_FAILURE); > - } else if (rc != SA_AIS_OK) { > + fprintf(stderr, "invoking clmprint from non-member > node!\n"); > + } > > [Zoran] Same here... double error messages > > + > + if (rc != SA_AIS_OK) { > fprintf(stderr, "error - clmprint:: saClmClusterTrack_4 > failed, rc = %d\n", rc); > exit(EXIT_FAILURE); > } > @@ -505,9 +506,10 @@ int main(int argc, char *argv[]) > case 'm': > rc = saClmClusterTrack_4(clm_handle, trackFlags, NULL); > if (rc == SA_AIS_ERR_UNAVAILABLE) { > - fprintf(stdout, "error - invoking clmprint from non- > member node\n"); > - exit(EXIT_FAILURE); > - } else if (rc != SA_AIS_OK) { > + fprintf(stderr, "invoking clmprint from non-member > node!\n"); > + } > > [Zoran] ... and here > > Thanks, > Zoran > > + > + if (rc != SA_AIS_OK) { > fprintf(stderr, "error - clmprint:: saClmClusterTrack_4 > failed, rc = %d\n", rc); > exit(EXIT_FAILURE); > } > -- > 1.9.1 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
