[v3,03/12] qga: move string split in separate function

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The function is going to be reused in a later patch.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/main.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

On 08/26/2015 01:05 PM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> The function is going to be reused in a later patch.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>   qga/main.c | 33 ++++++++++++++++++++++-----------
>   1 file changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/qga/main.c b/qga/main.c
> index 10bb2f7..e75022c 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -921,6 +921,26 @@ static void ga_print_cmd(QmpCommand *cmd, void *opaque)
>       printf("%s\n", qmp_command_name(cmd));
>   }
>   
> +static GList *split_list(gchar *str, const gchar separator)
> +{
it would be much better to declare this helper as "split_list(const 
gchar *str, ...
and make temporary copy of parameter inside. Without this the function
as NASTY side-effect and trashes the string passed in.

Also it is rather reinvents strtok wheel.

you can also use g_strsplit at your convenience, but may be this is overkill

> +    GList *list = NULL;
> +    int i, j, len;
> +
> +    for (j = 0, i = 0, len = strlen(str); i < len; i++) {
> +        if (str[i] == separator) {
> +            str[i] = 0;
> +            list = g_list_append(list, &str[j]);
> +            j = i + 1;
> +        }
> +    }
> +
> +    if (j < i) {
> +        list = g_list_append(list, &str[j]);
> +    }
> +
> +    return list;
> +}
> +
>   int main(int argc, char **argv)
>   {
>       const char *sopt = "hVvdm:p:l:f:F::b:s:t:";
> @@ -953,7 +973,7 @@ int main(int argc, char **argv)
>           { "statedir", 1, NULL, 't' },
>           { NULL, 0, NULL, 0 }
>       };
> -    int opt_ind = 0, ch, daemonize = 0, i, j, len;
> +    int opt_ind = 0, ch, daemonize = 0;
>       GLogLevelFlags log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
>       GList *blacklist = NULL;
>       GAState *s;
> @@ -1001,16 +1021,7 @@ int main(int argc, char **argv)
>                   qmp_for_each_command(ga_print_cmd, NULL);
>                   exit(EXIT_SUCCESS);
>               }
> -            for (j = 0, i = 0, len = strlen(optarg); i < len; i++) {
> -                if (optarg[i] == ',') {
> -                    optarg[i] = 0;
> -                    blacklist = g_list_append(blacklist, &optarg[j]);
> -                    j = i + 1;
> -                }
> -            }
> -            if (j < i) {
> -                blacklist = g_list_append(blacklist, &optarg[j]);
> -            }
> +            blacklist = g_list_concat(blacklist, split_list(optarg, ','));
>               break;
>           }
>   #ifdef _WIN32

Hi

On Wed, Aug 26, 2015 at 7:55 PM, Denis V. Lunev <den-lists@parallels.com> wrote:
> it would be much better to declare this helper as "split_list(const gchar
> *str, ...
> and make temporary copy of parameter inside. Without this the function
> as NASTY side-effect and trashes the string passed in.
> Also it is rather reinvents strtok wheel.

Yep, notice I didn't change the code, merely moved it.

> you can also use g_strsplit at your convenience, but may be this is overkill

But it doesn't return a glist. So while I agree with you in general,
that wasn't my goal to change this code, but just to be able to reuse
it.

thanks

On 08/26/2015 09:07 PM, Marc-André Lureau wrote:
> Hi
>
> On Wed, Aug 26, 2015 at 7:55 PM, Denis V. Lunev <den-lists@parallels.com> wrote:
>> it would be much better to declare this helper as "split_list(const gchar
>> *str, ...
>> and make temporary copy of parameter inside. Without this the function
>> as NASTY side-effect and trashes the string passed in.
>> Also it is rather reinvents strtok wheel.
> Yep, notice I didn't change the code, merely moved it.
I think that this side effect is visible if the code remains in place
and becomes invisible since you move it to the function.
This could create problem if somebody will reuse this call.


>> you can also use g_strsplit at your convenience, but may be this is overkill
> But it doesn't return a glist. So while I agree with you in general,
> that wasn't my goal to change this code, but just to be able to reuse
> it.
>
> thanks
>

Hi

On Wed, Aug 26, 2015 at 8:23 PM, Denis V. Lunev <den-lists@parallels.com> wrote:
> I think that this side effect is visible if the code remains in place
> and becomes invisible since you move it to the function.
> This could create problem if somebody will reuse this call.

what about replacing it with:

static GList *split_list(gchar *str, const gchar *delim)
{
    GList *list = NULL;
    int i;
    gchar **strv;

    strv = g_strsplit(str, delim, -1);
    for (i = 0; strv[i]; i++) {
        list = g_list_prepend(list, strv[i]);
    }
    g_free(strv);

    return list;
}

would that work for you?

the list must then be g_list_free_full()

Hi

On Wed, Aug 26, 2015 at 8:30 PM, Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
> the list must then be g_list_free_full()


Actually, the ga_command_blacklist_init() doesn't dup the string, so
it will have to do it too..tbh, I don't mind either way.

On 08/26/2015 09:30 PM, Marc-André Lureau wrote:
> Hi
>
> On Wed, Aug 26, 2015 at 8:23 PM, Denis V. Lunev <den-lists@parallels.com> wrote:
>> I think that this side effect is visible if the code remains in place
>> and becomes invisible since you move it to the function.
>> This could create problem if somebody will reuse this call.
> what about replacing it with:
>
> static GList *split_list(gchar *str, const gchar *delim)
> {
>      GList *list = NULL;
>      int i;
>      gchar **strv;
>
>      strv = g_strsplit(str, delim, -1);
>      for (i = 0; strv[i]; i++) {
>          list = g_list_prepend(list, strv[i]);
>      }
>      g_free(strv);
>
>      return list;
> }
>
> would that work for you?
yep! and you could declare it with 'const gchar *str'

> the list must then be g_list_free_full()
>