[Spice-devel] [PATCH 2/4] vdagent: move file xfer initialization to a function

[Pavel Grunt]

On Fri, 2017-05-12 at 09:33 -0400, Frediano Ziglio wrote:
> > 
> > On Fri, 2017-05-12 at 13:19 +0200, Victor Toso wrote:
> > > From: Victor Toso <me at victortoso.com>
> > > 
> > > This patch creates two functions:
> > > - xfer_get_download_directory()
> > > - vdagent_start_file_xfer()
> > > 
> > > The logic should be similar as it was before this patch, taking
> > > in
> > > consideration the global variables fx_open_dir and fx_dir which
> > > are
> > > set from command line.
> > > 
> > > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > > ---
> > >  src/vdagent/vdagent.c | 76
> > > +++++++++++++++++++++++++++++++++++++---
> > > -----------
> > >  1 file changed, 56 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
> > > index 9900303..08ebd92 100644
> > > --- a/src/vdagent/vdagent.c
> > > +++ b/src/vdagent/vdagent.c
> > > @@ -25,6 +25,7 @@
> > >  
> > >  #include <stdio.h>
> > >  #include <stdlib.h>
> > > +#include <stdbool.h>
> > >  #include <string.h>
> > >  #include <syslog.h>
> > >  #include <unistd.h>
> > > @@ -55,6 +56,59 @@ static struct udscs_connection *client =
> > > NULL;
> > >  static int quit = 0;
> > >  static int version_mismatch = 0;
> > >  
> > > +/**
> > > + * xfer_get_download_directory
> > > + *
> > > + * Return path were file-transfer will save the files. Should
> > > be
> > > freed with
> > > + * g_free().
> > > + **/
> > > +static char *
> > 
> > should be g_char (g_strdup returns g_char *)
> > Is anything preventing to return const gchar * ?
> > 
> 
> I think free functions (like g_free) don't like const that much.

of course, g_free calls have to be removed

> 
> > > +xfer_get_download_directory(bool *open_dir_on_save)
> > > +{
> > > +    if (open_dir_on_save != NULL)
> > > +        *open_dir_on_save = fx_open_dir != -1 ||
> > > !vdagent_x11_has_icons_on_desktop(x11);
> > > +
> > > +    if (fx_dir)
> > 
> > I prefer an explicit comparison to NULL (like you do in the
> > previous
> > 'if')
> > > +        return g_strdup(fx_dir);
> > > +
> > > +    if (vdagent_x11_has_icons_on_desktop(x11))
> > > +        return
> > > g_strdup(g_get_user_special_dir(G_USER_DIRECTORY_DESKTOP));
> > > +
> > > +    return
> > > g_strdup(g_get_user_special_dir(G_USER_DIRECTORY_DOWNLOAD));
> > > +}
> > > +
> > > +/**
> > > + * vdagent_start_file_xfer
> > > + *
> > > + * Initialize handler for file xfer and returns true if
> > > vdagent_file_xfers is
> > 
> > Why 'start' ? it does not start anything
> > 
> > > + * not NULL.
> > > + **/
> > > +static bool
> > > +vdagent_start_file_xfer(void)
> > > +{
> > > +    bool xfer_open_dir;
> > > +    char *xfer_dir;
> > > +
> > > +    if (vdagent_file_xfers != NULL) {
> > > +        syslog(LOG_WARNING, "File-xfer already initialized");
> > > +        return true;
> > > +    }
> > > +
> > > +    xfer_dir = xfer_get_download_directory(&xfer_open_dir);
> > > +    if (xfer_dir == NULL) {
> > > +        syslog(LOG_WARNING,
> > > +               "warning could not get file xfer save dir, "
> > > +               "file transfers will be disabled");
> > > +        vdagent_file_xfers = NULL;
> > > +        return false;
> > > +    }
> > > +
> > > +    vdagent_file_xfers = vdagent_file_xfers_create(client,
> > > xfer_dir,
> > > +                                                   xfer_open_di
> > > r,
> > > debug);
> > > +    g_free(xfer_dir);
> 
> This should generate a warning if xfer_dir is a "const XXXX *".
> 
> ...
> 
> Frediano