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

[Victor Toso]

Hi,

On Mon, May 29, 2017 at 03:05:49PM +0200, Christophe Fergeau wrote:
> On Mon, May 29, 2017 at 02:43:26PM +0200, Victor Toso wrote:
> > 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.
>
> But it would be a programming error, not something that the
> user/sysadmin can do anything about.

Yeah, besides filing bugs :)

> So g_return_val_if_fail() might be better here?
>
> Christophe

Not sure how this will be handled without stdin/stdout? Likely it'll be
ignored? I'll check but I don't mind moving to DEBUG for now.

Thanks for the reviews,
    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/20170529/19808153/attachment.sig>