[winswitch] Why use user provided log file only when display_name is set?

[Antoine Martin]

On 18/03/17 05:34, Timothy Hobbs via shifter-users wrote:
> Hi,
> 
> I'm reading the xpra code now, and I don't understand the following
> condition:
> 
> 668    def select_log_file(log_dir, log_file, display_name):
> 669        """ returns the log file path we should be using given the
> parameters,
> 670            this may return a temporary logpath if display_name is
> not available.
> 671        """
> 672        if log_file and display_name:
> 673            if os.path.isabs(log_file):
> 674                logpath = log_file
> 675            else:
> 676                logpath = os.path.join(log_dir, log_file)
> 677            logpath = shellsub(logpath, {"DISPLAY" : display_name})
> 678        elif display_name:
> 679            logpath = norm_makepath(log_dir, display_name) + ".log"
> 680        else:
> 681            logpath = os.path.join(log_dir, "tmp_%d.log" % os.getpid())
> 682        return logpath
> 
> https://www.xpra.org/trac/browser/xpra/trunk/src/xpra/scripts/server.py#L672
> 
> 
> On line 672 the condition is that display_name must be set in order for
> the user set log_file option to be used. However, looking at the code, I
> see no reason for this to be the case. So why is it needed?
We're trying to avoid using a log filename which has "${DISPLAY}" in it
since it would get substituted with "None" until we actually have the
display name available.
This would be a bad thing because it would make the code racy if you
tried to start two servers at the same time.

Some links that may be relevant since you're looking at this code:
* original displayfd feature patch review:
http://xpra.org/trac/ticket/172#comment:15
* Xorg 1.18.1 log file renaming bug (upstream, unfixed):
http://xpra.org/trac/ticket/1192
https://bugs.freedesktop.org/show_bug.cgi?id=95234

Cheers
Antoine

If anything, this would be more correct to apply (untested).
If you can review this and it works, it should be safe to apply:
===================================================================
--- xpra/scripts/server.py	(revision 15316)
+++ xpra/scripts/server.py	(working copy)
@@ -669,13 +669,16 @@
     """ returns the log file path we should be using given the parameters,
         this may return a temporary logpath if display_name is not
available.
     """
-    if log_file and display_name:
+    if log_file:
         if os.path.isabs(log_file):
             logpath = log_file
         else:
             logpath = os.path.join(log_dir, log_file)
-        logpath = shellsub(logpath, {"DISPLAY" : display_name})
-    elif display_name:
+        v = shellsub(logpath, {"DISPLAY" : display_name})
+        if display_name or v==logpath:
+            #we have 'display_name', or we just don't need it:
+            return v
+    if display_name:
         logpath = norm_makepath(log_dir, display_name) + ".log"
     else:
         logpath = os.path.join(log_dir, "tmp_%d.log" % os.getpid())




> 
> Why not just
> 
> 672        if log_file:
> 673            if os.path.isabs(log_file):
> 674                logpath = log_file
> ....
> 
> 
> Regards,
> 
> Timothy Hobbs
> 
> _______________________________________________
> shifter-users mailing list
> shifter-users at lists.devloop.org.uk
> http://lists.devloop.org.uk/mailman/listinfo/shifter-users