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