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

[Victor Toso]

Hi,

On Wed, May 17, 2017 at 06:29:06PM +0200, Christophe Fergeau wrote:
> On Tue, May 16, 2017 at 07:27:53PM +0200, Victor Toso wrote:
> > From: Victor Toso <me at victortoso.com>
> > 
> > This patch creates two functions:
> > - xfer_get_download_directory()
> > - vdagent_init_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 | 73 +++++++++++++++++++++++++++++++++++++--------------
> >  1 file changed, 53 insertions(+), 20 deletions(-)
> > 
> > diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
> > index 9900303..d1e67a2 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,56 @@ 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 const gchar *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);
> 
> This assignment is not very easy to read... Does this have to be in
> xfer_get_download_directory? I find this unrelated to that function.

Indeed, I'll put it in a different function/place.

>
> > +
> > +    if (fx_dir != NULL)
> > +        return fx_dir;
> > +
> > +    if (vdagent_x11_has_icons_on_desktop(x11))
> > +        return g_get_user_special_dir(G_USER_DIRECTORY_DESKTOP);
> > +
> > +    return g_get_user_special_dir(G_USER_DIRECTORY_DOWNLOAD);
> > +}
> > +
> > +/**
> > + * vdagent_init_file_xfer
> > + *
> > + * Initialize handler for file xfer and returns true if vdagent_file_xfers is
> > + * not NULL.
> > + **/
> > +static bool vdagent_init_file_xfer(void)
> > +{
> > +    bool xfer_open_dir;
> > +    const gchar *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_dir, debug);
> > +    return (vdagent_file_xfers != NULL);
> > +}
> > +
> >  static void daemon_read_complete(struct udscs_connection **connp,
> >      struct udscs_message_header *header, uint8_t *data)
> >  {
> > @@ -313,26 +364,8 @@ reconnect:
> >          return 1;
> >      }
> >  
> > -    if (!fx_dir) {
> > -        if (vdagent_x11_has_icons_on_desktop(x11))
> > -            fx_dir = "xdg-desktop";
> > -        else
> > -            fx_dir = "xdg-download";
> > -    }
> > -    if (fx_open_dir == -1)
> > -        fx_open_dir = !vdagent_x11_has_icons_on_desktop(x11);
> > -    if (!strcmp(fx_dir, "xdg-desktop"))
> > -        fx_dir = g_get_user_special_dir(G_USER_DIRECTORY_DESKTOP);
> > -    else if (!strcmp(fx_dir, "xdg-download"))
> > -        fx_dir = g_get_user_special_dir(G_USER_DIRECTORY_DOWNLOAD);
> 
> Note that before the help string was:
> -f <dir|xdg-desktop|xdg-download> file xfer save dir
> 
> ie 'spice-vdagent -f xdg-desktop' was a valid option, and would 
> resolve to g_get_user_special_dir(G_USER_DIRECTORY_DESKTOP);
> 
> Dunno if we want to keep that.
> 
> Christophe

For me it is quite redundant as -f can set any path and automatically on
x11, if it has icons on desktop it will use G_USER_DIRECTORY_DESKTOP.

Let me know if you (or someone else) thinks this bits are important to
keep.

Thanks for the review,
    toso
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170518/e8404716/attachment-0001.sig>