Date: Thu, 25 Apr 2013 14:08:51 -0400 From: jferlan(a)redhat.com To: libvirt-cim(a)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.

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"

I would also think the comma would be outside the quotes, but don't have experience with the API in question.

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?

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"

> 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.

> + 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 "{}"?

> // 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()

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(a)redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim