<html> <head> <style></style></head> <body class='hmmessage'><div dir='ltr'> <div><div id="SkyDrivePlaceholder"></div>> Date: Thu, 25 Apr 2013 14:08:51 -0400 > From: jferlan@redhat.com > To: libvirt-cim@redhat.com > Subject: Re: [Libvirt-cim] [PATCH 3/3] make lldptool command and support output configurable > > > For some reason the original email never made it into my inbox, so I've > had to cut-n-paste from the mail list archives and I hope I picked the > right message-id for the messages to line up properly!!! > > > > > From: Xu Wang <cngesaint outlook com> > > > > There's no commit message? > > > Signed-off-by: Xu Wang <cngesaint outlook com> > > --- > > libvirt-cim.conf | 14 ++++++++++ > > libxkutil/misc_util.c | 18 +++++++++++++ > > libxkutil/misc_util.h | 2 + > > src/Virt_SwitchService.c | 63 ++++++++++++++++++++++++++++++++++++++-------- > > 4 files changed, 86 insertions(+), 11 deletions(-) > > > > diff --git a/libvirt-cim.conf b/libvirt-cim.conf > > index 3244ee3..396dac9 100644 > > --- a/libvirt-cim.conf > > +++ b/libvirt-cim.conf > > @@ -38,3 +38,17 @@ > > # Default value: false > > # > > # force_use_qemu = false; > > + > > +# lldptool_query_options (string) > > +# Defines the command used in SwitchService to query VEPA support, will be > > +# used as "lldptool -i [INTERFACE] [OPTIONS]" > > +# > > +# lldptool_query_options = "-t -g ncb -V evbcfg"; > > + > > +# vsi_support_key_string (string) > > +# Defines the string used in SwitchService to search in lldptool's output > > +# When lldptool updates its output, please update the new output into the > > +# output set. add comma between items. > > +# > > +# vsi_support_key_string = "{ supported forwarding mode: (0x40) reflective relay," > > +# " supported capabilities: (0x7) RTE ECP VDP}"; > > The code later indicates all the strings have to be there; however, the > comments here do not match that. There's also no mention here that at > most 8 lines can be added. Hopefully we never need to go beyond that > number (however arbitrary 8 is). Is the semicolon mandatory? Seems to > me whatever format is required needs to be well documented. I'll supplement about format of vsi_support_key_string and limit of items about it into comment. > > With respect to the strings and the parsing done, should I 'assume' that > the config_lookup_string() will know how to read the config file where > elements span multiple lines.... In particular, is > > "line1," > "line2" > > read/returned as "line1,line2"? > > I would think a continuation "\" marker would be necessary: > > "line1," \ > "line2" It's OK without slash (It was successful under my test). I haven't test for the format like above. > > I would also think the comma would be outside the quotes, but don't have > experience with the API in question. The application read vsi_support_key_string as a whole string and use comma to slice them (e.g.,above string will be devided into 2 parts, one before comma and the other one after comma) > > Since I don't have a way to test this - I'm curious while coding this up > if you checked that both lines were read when parsing? You can adjust value of vsi_support_key_string and then the output of CU_DEBUG in the log would changed as your change. I made every item would write into /var/log/libvirt-cim/debug.txt > > Secondarily, there's some extraneous spaces which perhaps need to be > cleaned up. Since the code later on will separate based on ",". What > happens if message #3 comes along some day as: > > "support feature1, feature2, and feature3: (0x83) mumbly fratz" I took many cases into consideration. At last I think all special symbols are not safe because all of them could appear in the future output. So I picked ",". If someday "," appeared in output, there are just two places need to be updated. Firstly, delimiter set in the variable delim (now is "," "{" and "}"). Secondly, content of vsi_support_key_string and its comments. If you have any better suggestions please let me know. > > > > > > diff --git a/libxkutil/misc_util.c b/libxkutil/misc_util.c > > index 4c0b0a1..6ce8dca 100644 > > --- a/libxkutil/misc_util.c > > +++ b/libxkutil/misc_util.c > > @@ -244,6 +244,24 @@ const char *get_mig_ssh_tmp_key(void) > > return prop.value_string; > > } > > > > +const char *get_lldptool_query_options(void) > > +{ > > + static LibvirtcimConfigProperty prop = { > > + "lldptool_query_options", CONFIG_STRING, {0}, 0}; > > + > > + libvirt_cim_config_get(&prop); > > + return prop.value_string; > > +} > > + > > +const char *get_vsi_support_key_string(void) > > +{ > > + static LibvirtcimConfigProperty prop = { > > + "vsi_support_key_string", CONFIG_STRING, {0}, 0};; > > + > > + libvirt_cim_config_get(&prop); > > + return prop.value_string; > > +} > > + > > virConnectPtr connect_by_classname(const CMPIBroker *broker, > > const char *classname, > > CMPIStatus *s) > > diff --git a/libxkutil/misc_util.h b/libxkutil/misc_util.h > > index 9e6b419..4eb588d 100644 > > --- a/libxkutil/misc_util.h > > +++ b/libxkutil/misc_util.h > > @@ -155,6 +155,8 @@ int virt_set_status(const CMPIBroker *broker, > > /* get libvirt-cim config */ > > const char *get_mig_ssh_tmp_key(void); > > bool get_force_use_qemu(void); > > +const char *get_lldptool_query_options(void); > > +const char *get_vsi_support_key_string(void); > > > > /* > > * Local Variables: > > diff --git a/src/Virt_SwitchService.c b/src/Virt_SwitchService.c > > index 8991426..f7bafbf 100644 > > --- a/src/Virt_SwitchService.c > > +++ b/src/Virt_SwitchService.c > > @@ -46,12 +46,26 @@ static CMPIStatus check_vsi_support(char *command) > > CMPIStatus s = {CMPI_RC_OK, NULL}; > > char buff[MAX_LEN]; > > FILE *stream = NULL; > > - const char *searchStr[] = {" supported forwarding mode: " > > - "(0x40) reflective relay", > > - " supported capabilities: " > > - "(0x07) RTE ECP VDP", > > - NULL}; > > - int matched = 0; > > + char *searchStr[8]; /* maximum items of vsi support output */ > > + int count = 0; > > + const char *user_settings = get_vsi_support_key_string(); > > + char *vsi_support_key_string = NULL; > > + char *delim = "{},"; > > + int matched = 0; > > + char *temp = NULL; > > + > > + if (!user_settings) { > > + user_settings = "{ supported forwarding mode: " > > + "(0x40) reflective relay," > > + " supported capabilities: " > > + "(0x7) RTE ECP VDP}"; > > + } > > + > > + vsi_support_key_string = strdup(user_settings); > > There's a memory leak hear if 'user_settings' is returned from the api. > That code will strdup() whatever is returned, it's returned here, > strdup()'d again and leaked. Yes, so it is. > > > > + if (vsi_support_key_string == NULL) { > > + CU_DEBUG("strdup vsi_support_key_string failed!"); > > I think you need to call cu_statusf here; otherwise, 's' returns as > {CMPI_RC_OK, NULL} > > > + goto out; > > + } > > > > // Run lldptool command to find vsi support. > > stream = popen(command, "r"); > > @@ -63,6 +77,25 @@ static CMPIStatus check_vsi_support(char *command) > > goto out; > > } > > > > + /* Slice vsi_support_key_string into items */ > > + searchStr[count] = strtok_r(vsi_support_key_string, delim, &temp); > > + if (searchStr[count] == NULL) { > > + CU_DEBUG("searchStr fetch failed when calling strtok_r!"); > > + } else { > > + CU_DEBUG("searchStr[%d]: %s", count, searchStr[count]); > > + count++; > > + } > > + > > + while ((searchStr[count] = strtok_r(NULL, delim, &temp))) { > > + if (count >= 7) { > > + CU_DEBUG("WARN: searchStr is full, left aborted!"); > > + break; > > + } else { > > + CU_DEBUG("searchStr[%d]: %s", count, searchStr[count]); > > + count++; > > + } > > + } > > + > > What if count == 0? That is can the element in the conf file be "{}"? Because the condition of vsi_support_key_string was set in the configuration file is the default output items for vsi support check need to be updated. So the level of vsi_support_key_string is higher than default. That's the use of keyword in the .conf file. So if a void content in the .conf file is not needed and should be commented out. I'll add this point into comment of .conf file. > > > // Read the output of the command. > > while (fgets(buff, MAX_LEN, stream) != NULL) { > > int i = 0; > > @@ -81,16 +114,18 @@ static CMPIStatus check_vsi_support(char *command) > > } > > /* All the search strings were found in the output of this > > command. */ > > - if (matched == 2) { > > + if (matched == count) { > > cu_statusf(_BROKER, &s, CMPI_RC_OK, "VSI supported"); > > - goto out;; > > + goto out; > > } > > } > > + > > cu_statusf(_BROKER, &s, > > CMPI_RC_ERR_NOT_FOUND, > > "No VSI Support found"); > > > > - out: > > + out: > > + free(vsi_support_key_string); > > if (stream != NULL) > > pclose(stream); > > return s; > > @@ -214,6 +249,7 @@ static CMPIStatus get_switchservice(const CMPIObjectPath *reference, > > int i; > > char **if_list; > > char cmd[MAX_LEN]; > > + const char *lldptool_query_options = NULL; > > > > *_inst = NULL; > > conn = connect_by_classname(broker, CLASSNAME(reference), &s); > > @@ -257,10 +293,15 @@ static CMPIStatus get_switchservice(const CMPIObjectPath *reference, > > > > CU_DEBUG("Found %d interfaces", count); > > > > + lldptool_query_options = get_lldptool_query_options(); > > + if (!lldptool_query_options) { > > + lldptool_query_options = "-t -g ncb -V evbcfg"; > > + } > > Memory leak if we are successful from get_lldptool_query_options() free() will be added. > > John > > > > > for (i=0; i<count; i++) { > > - sprintf(cmd, "lldptool -i %s -t -V evbcfg", if_list[i]); > > - CU_DEBUG("running command %s ...", cmd); > > + sprintf(cmd, "lldptool -i %s %s", > > + if_list[i], lldptool_query_options); > > + CU_DEBUG("running command [%s]", cmd); > > s = check_vsi_support(cmd); > > if (s.rc == CMPI_RC_OK) { > > vsi = true; > > -- > > 1.7.1 > > _______________________________________________ > Libvirt-cim mailing list > Libvirt-cim@redhat.com > https://www.redhat.com/mailman/listinfo/libvirt-cim </div> </div></body> </html>