[Spice-devel] [PATCH v3 1/3] vdagent: move file xfer initialization to a function

[Victor Toso]

Hi,

On Mon, May 29, 2017 at 01:31:35PM +0200, Christophe Fergeau wrote:
> On Thu, May 18, 2017 at 12:21:55PM +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.
>
> Fwiw, this is not true as this removes handling of
> "xdg-download"/"xdg-desktop" from the command line. To prevent confusion
> in the future/make a revert easier if needed/..., I would split this
> removal in a separate patch (or handle these magic values in
> xfer_get_download_directory())

Will do

>
> >
> > 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..03f8021 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(void)
> > +{
> > +    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");
>
> Does this deserves a syslog warning?

Hmmm, Maybe?

If we take into account only the current code, I would say no, DEBUG is
plenty. But overall, trying to _init() something twice should be more
verbose, IMO.

>
> > +        return true;
> > +    }
> > +
> > +    xfer_dir = xfer_get_download_directory();
> > +    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;
> > +    }
> > +
> > +    /* Open directory either if requested or target path is not xdg-desktop */
> > +    xfer_open_dir = fx_open_dir != -1 || !vdagent_x11_has_icons_on_desktop(x11);
>
> This is hard to parse, and I'm not sure it's correct when '-o 0' is
> used.
>
>     if (fx_open_dir != -1) {
>         xfer_open_dir = fx_open_dir;
>     } else {
>         xfer_open_dir = !vdagent_x11_has_icons_on_desktop(x11);
>     }
>
> should work.

Sure

>
> Apart from this,
> Acked-by: Christophe Fergeau <cfergeau at redhat.com>
>
> Christophe

Thank, I'll split the changes and send another version

JFYI, I'm playing with agent code to make it possible to integrate with
gtk at some point. Patches are not polished but vdagent is working with
GMainLoop in 'mainloop' branch at: https://gitlab.com/victortoso/vdagent

Cheers,
-------------- 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/20170529/9ecb91dd/attachment.sig>