> 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