Re: [Libreoffice-commits] osl_areCommandArgsSet

* osl_areCommandArgsSet should arguably have been given a return type of 
sal_Bool, not int

* the documentation of osl_areCommandArgsSet is missing a @since tag; 
please fix

* given that osl_setCommandArgs is deprecated and for internal use only, 
osl_areCommandArgsSet should arguably have been added as internal-only 
functionality ("detail" identifier namespace, "PRIVATE_" ELF symbol map 
namespace); please at least mark it as internal-only in the documentation

* the implementations of osl_areCommandArgsSet 
(sal/osl/unx/process_impl.cxx, sal/osl/w32/process.cxx) are broken as 
they access g_command_args.m_nCount without the corresponding mutex 
locked; please fix

* the design of osl_areCommandArgsSet and its use in 
desktop/source/lib/init.cxx are broken, as it is prone to TOCTOU

* adding osl_areCommandArgsSet was unnecessary; given that 
osl_setCommandArgs(0,NULL) is already handled as a special case, that 
case can be extended to silently do nothing if osl_setCommandArgs has 
already been called previously; please fix the LOK use-case that way

* "and possibly if UNO is being used in addition to LOK in an external 
program": by design, osl_setCommandsArgs is exclusively called from 
sal_detail_initialize, which in turn is exclusively called form 
SAL_IMPLEMENT_MAIN[_WITH_ARGS], and every process that uses binary UNO 
needs to implement main via those macros; note that 
sal_detail_[de]initialize potentially does further things necessary for 
binary UNO to work properly besides calling osl_setCommandArgs; it is 
unwise to hack around that for the LOK use-case in an ad hoc way

* was it necessary to backport osl_areCommandArgsSet to LO 4.3 (given I 
see no mention of LOK in 
<https://wiki.documentfoundation.org/ReleaseNotes/4.3>)?

Stephan

On 07/10/2014 08:17 PM, Andrzej Hunt wrote:
> Rebased ref, commits from common ancestor:
> commit aef3fcd39558e424b816e5eb07a9421ab046ff0f
> Author: Andrzej Hunt <andrzej.hunt@collabora.com>
> Date:   Thu Jul 10 12:19:36 2014 +0200
>
>      Check whether Command Args are already set up before doing so.
>
>      Could already be set up e.g. if a client application is using UNO
>      separately, in addition to LOK.
>
>      Change-Id: I50c3230b6f2456360273902a308c303576baac10
>
> diff --git a/desktop/source/lib/init.cxx b/desktop/source/lib/init.cxx
> index e1931b5..520d7ca 100644
> --- a/desktop/source/lib/init.cxx
> +++ b/desktop/source/lib/init.cxx
> @@ -607,7 +607,14 @@ static int lo_initialize(LibreOfficeKit* pThis, const char* pAppPath)
>
>       try
>       {
> -        osl_setCommandArgs(0, NULL);
> +        // If we've set up the command args elsewhere then we cannot do it
> +        // again (as an assert will fire), this will be the case e.g.
> +        // for unit tests (and possibly if UNO is being used in addition
> +        // to LOK in an external program).
> +        if (!osl_areCommandArgsSet())
> +        {
> +            osl_setCommandArgs(0, NULL);
> +        }
>           initialize_uno(aAppURL);
>           force_c_locale();
>
> commit 91e852a36d57e514dd4b1fb5421e9e5e19172ea4
> Author: Andrzej Hunt <andrzej.hunt@collabora.com>
> Date:   Thu Jul 10 12:17:05 2014 +0200
>
>      Introduce osl_areCommandArgsSet.
>
>      We cannot call osl_setCommandArgs twice, however there is currently
>      no way to determine whether or not this has already been done. This is
>      necessary e.g. for LibreOfficeKit where we may also be using UNO
>      separately (and also for unit tests where LO is already set-up prior
>      to the unit test running, and therefore we can't set up osl again
>      from within LOK).
>
>      Change-Id: Id1f357ef604eb2b6b7814c9a04ac6933a39fd3eb
>
> diff --git a/include/osl/process.h b/include/osl/process.h
> index 7d0960e..1a9a416 100644
> --- a/include/osl/process.h
> +++ b/include/osl/process.h
> @@ -366,6 +366,11 @@ SAL_DLLPUBLIC sal_uInt32 SAL_CALL osl_getCommandArgCount(void);
>   SAL_DLLPUBLIC oslProcessError SAL_CALL osl_getCommandArg(
>           sal_uInt32 nArg, rtl_uString **strCommandArg);
>
> +/** Determine whether or not the command args have already been set.
> +    @return The command args are already set, and may not be set again.
> +*/
> +SAL_DLLPUBLIC int SAL_CALL osl_areCommandArgsSet ();
> +
>   /** Set the command-line arguments as passed to the main-function of this process.
>
>       Deprecated: This function is only for internal use. Passing the args from main will
> diff --git a/sal/osl/unx/process_impl.cxx b/sal/osl/unx/process_impl.cxx
> index d28f46d..b63f222 100644
> --- a/sal/osl/unx/process_impl.cxx
> +++ b/sal/osl/unx/process_impl.cxx
> @@ -191,6 +191,11 @@ oslProcessError SAL_CALL osl_getCommandArg (sal_uInt32 nArg, rtl_uString ** 
> strC
>       return (result);
>   }
>
> +int SAL_CALL osl_areCommandArgsSet (void)
> +{
> +    return (g_command_args.m_nCount > 0);
> +}
> +
>   /***************************************
>    osl_setCommandArgs().
>    **************************************/
> diff --git a/sal/osl/w32/process.cxx b/sal/osl/w32/process.cxx
> index 3dd0e77..25f4e58 100644
> --- a/sal/osl/w32/process.cxx
> +++ b/sal/osl/w32/process.cxx
> @@ -374,6 +374,12 @@ oslProcessError SAL_CALL osl_getCommandArg( sal_uInt32 nArg, rtl_uString 
> **strCo
>
>   /***************************************************************************/
>
> +int SAL_CALL osl_areCommandArgsSet(void)
> +{
> +    return (g_command_args.m_nCount > 0);
> +}
> +
> +
>   void SAL_CALL osl_setCommandArgs (int argc, char ** argv)
>   {
>       osl_acquireMutex (*osl_getGlobalMutex());
> diff --git a/sal/util/sal.map b/sal/util/sal.map
> index 1d7d491..6e95369 100644
> --- a/sal/util/sal.map
> +++ b/sal/util/sal.map
> @@ -677,6 +677,12 @@ LIBO_UDK_4.3 { # symbols available in >= LibO 4.3
>           rtl_freeAlignedMemory;
>   } LIBO_UDK_4.2;
>
> +LIBO_UDK_4.4 { # symbols available in >= LibO 4.4
> +    global:
> +        osl_areCommandArgsSet;
> +} LIBO_UDK_4.3;
> +
> +
>   PRIVATE_1.0 {
>       global:
>           osl_detail_ObjectRegistry_storeAddresses;