[REVIEW] Null pointer passed as an argument to a 'nonnull' parameter in vcl/unx/generic/app/i18n_wrp.cxx

classic Classic list List threaded Threaded
16 messages Options
julien2412 julien2412
Reply | Threaded
Open this post in threaded view
|

[REVIEW] Null pointer passed as an argument to a 'nonnull' parameter in vcl/unx/generic/app/i18n_wrp.cxx

Hello,

By keeping on reading the logs cclang compilation, I noticed this :
/home/julien/compile-libreoffice/libo/vcl/unx/generic/app/i18n_wrp.cxx:250:9: warning: Null pointer passed as an argument to a 'nonnull' parameter
        dlclose(g_dlmodule);
        ^       ~~~~~~~~~~
1 warning generated.
Here are the lines :
    243 Status XvaCloseIM(XIM)
    244 {
    245       Status s = False;
    246
    247     if (!g_dlmodule)  <<<<< CAUSE OF THE PB
    248     {
    249         /* assuming one XvaOpenIM call */
    250         dlclose(g_dlmodule); <<<<< HERE
    251             g_dlmodule = (void*)0;
    252         g_open_im = (OpenFunction)NULL;
    253         s = True;
    254       }
    255     return (s);
    256 }
So here's an obvious patch :
diff --git a/vcl/unx/generic/app/i18n_wrp.cxx b/vcl/unx/generic/app/i18n_wrp.cxx
index 3aff25c..b95ad21 100644
--- a/vcl/unx/generic/app/i18n_wrp.cxx
+++ b/vcl/unx/generic/app/i18n_wrp.cxx
@@ -244,7 +244,7 @@ Status XvaCloseIM(XIM)
 {
       Status s = False;
 
-    if (!g_dlmodule)
+    if (g_dlmodule)
     {
         /* assuming one XvaOpenIM call */
         dlclose(g_dlmodule);

I suppose again it's ok to push this fix on master but would it be useful to push this on 3.5 branch too ?
Remark : when I opengroked this function , I found nothing. Is this function used (or should be used) in a way ?

Julien.
sberg sberg
Reply | Threaded
Open this post in threaded view
|

Re: [REVIEW] Null pointer passed as an argument to a 'nonnull' parameter in vcl/unx/generic/app/i18n_wrp.cxx

On 02/16/2012 11:39 PM, julien2412 wrote:
> /home/julien/compile-libreoffice/libo/vcl/unx/generic/app/i18n_wrp.cxx:250:9:
> warning: Null pointer passed as an argument to a 'nonnull' parameter
>          dlclose(g_dlmodule);
>          ^       ~~~~~~~~~~
> 1 warning generated.
> Here are the lines :
>      243 Status XvaCloseIM(XIM)
[...]
> Remark : when I opengroked this function , I found nothing. Is this function
> used (or should be used) in a way ?

Looks like all of vcl/unx/generic/app/i18n_wrp.cxx became completely
unused with
<http://cgit.freedesktop.org/libreoffice/core/commit/?id=8b0287543d87659fd4bfde5edb6725ee5da5f80e>
"These multi-lingual IMs have been always disabled for Linux."

So I would suggest to remove that code wholesale.

Stephan
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Tor Lillqvist-2 Tor Lillqvist-2
Reply | Threaded
Open this post in threaded view
|

Re: [REVIEW] Null pointer passed as an argument to a 'nonnull' parameter in vcl/unx/generic/app/i18n_wrp.cxx

> So I would suggest to remove that code wholesale.

Hmm, but was this code used on Solaris? Have we already removed some
functionality needed (or at least useful) on Solaris then? Too bad as
there recently has been people here talking about supporting Solaris?
(When I say "Solaris", I  mean whichever of them somebody wants to
support...)

--tml
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
sberg sberg
Reply | Threaded
Open this post in threaded view
|

Re: [REVIEW] Null pointer passed as an argument to a 'nonnull' parameter in vcl/unx/generic/app/i18n_wrp.cxx

On 02/17/2012 09:53 AM, Tor Lillqvist wrote:
>> So I would suggest to remove that code wholesale.
>
> Hmm, but was this code used on Solaris? Have we already removed some
> functionality needed (or at least useful) on Solaris then? Too bad as
> there recently has been people here talking about supporting Solaris?
> (When I say "Solaris", I  mean whichever of them somebody wants to
> support...)

But even then, leaving in just half of it while the rest has already
been removed would not make much sense IMO.

Stephan
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Tor Lillqvist-2 Tor Lillqvist-2
Reply | Threaded
Open this post in threaded view
|

Re: [REVIEW] Null pointer passed as an argument to a 'nonnull' parameter in vcl/unx/generic/app/i18n_wrp.cxx

> But even then, leaving in just half of it while the rest has already been
> removed would not make much sense IMO.

Indeed, so should the earlier removal be reverted?

--tml
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Michael Meeks-2 Michael Meeks-2
Reply | Threaded
Open this post in threaded view
|

Re: [REVIEW] Null pointer passed as an argument to a 'nonnull' parameter in vcl/unx/generic/app/i18n_wrp.cxx


On Fri, 2012-02-17 at 12:16 +0200, Tor Lillqvist wrote:
> > But even then, leaving in just half of it while the rest has already been
> > removed would not make much sense IMO.
>
> Indeed, so should the earlier removal be reverted?

        If people want to make Solaris (or it's more viable free software
brethren) work they're more than welcome.

        In the meantime, having un-tested, un-executed, legacy code around for
a platform we don't support seems pretty pointless to me :-)

        All the best,

                Michael.

--
[hidden email]  <><, Pseudo Engineer, itinerant idiot

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Tor Lillqvist-2 Tor Lillqvist-2
Reply | Threaded
Open this post in threaded view
|

Re: [REVIEW] Null pointer passed as an argument to a 'nonnull' parameter in vcl/unx/generic/app/i18n_wrp.cxx

> In the meantime, having un-tested, un-executed, legacy code around for
> a platform we don't support seems pretty pointless to me :-)

What message does that send to the Solaris porters?

--tml
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Michael Meeks-2 Michael Meeks-2
Reply | Threaded
Open this post in threaded view
|

Re: [REVIEW] Null pointer passed as an argument to a 'nonnull' parameter in vcl/unx/generic/app/i18n_wrp.cxx


On Fri, 2012-02-17 at 13:21 +0200, Tor Lillqvist wrote:
> > In the meantime, having un-tested, un-executed, legacy code around for
> > a platform we don't support seems pretty pointless to me :-)
>
> What message does that send to the Solaris porters?

        To pull their finger out and get it building on OpenIndiana / Illumos /
whatever quickish ? :-) I'm very happy to help do the (fairly trivial)
grep for 'SOLARIS' in the git log -u output - to find the bits that have
been removed that they might want to help restore it later if that's
what they want.

        All the best,

                Michael.

--
[hidden email]  <><, Pseudo Engineer, itinerant idiot

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Caolán McNamara Caolán McNamara
Reply | Threaded
Open this post in threaded view
|

Re: [REVIEW] Null pointer passed as an argument to a 'nonnull' parameter in vcl/unx/generic/app/i18n_wrp.cxx

In reply to this post by Tor Lillqvist-2
On Fri, 2012-02-17 at 10:53 +0200, Tor Lillqvist wrote:
> > So I would suggest to remove that code wholesale.
>
> Hmm, but was this code used on Solaris? Have we already removed some
> functionality needed (or at least useful) on Solaris then?

I doubt it, solaris had moved to a GNOME desktop by default, more or
less, when I left Sun and last used solaris and all of this is
irrelevant under the gtk vclplug.

It doesn't really make a lot of sense to retain direct support for one
archaic Input Mechanism which has been superseded, a few time over by
now, by a series of new IMs. IIIMP support probably shouldn't have been
made solaris-specific in the first place as e.g. fedora had it in up
until Fedora 5 in 2006 or so.

In fact we should probably go so far as to drop the "generic" vcl plug
altogether and just have the gtk and kde ones.

C.

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Michael Meeks-2 Michael Meeks-2
Reply | Threaded
Open this post in threaded view
|

Re: [REVIEW] Null pointer passed as an argument to a 'nonnull' parameter in vcl/unx/generic/app/i18n_wrp.cxx


On Fri, 2012-02-17 at 14:05 +0000, Caolán McNamara wrote:
> In fact we should probably go so far as to drop the "generic" vcl plug
> altogether and just have the gtk and kde ones.

        An interesting idea; might be nice to do, though of course, since so
much code is shared anyway between gtk2, kde and gen - I don't know how
much we'd actually save (?).

        ATB,

                Michael.

--
[hidden email]  <><, Pseudo Engineer, itinerant idiot

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Caolán McNamara Caolán McNamara
Reply | Threaded
Open this post in threaded view
|

Re: [REVIEW] Null pointer passed as an argument to a 'nonnull' parameter in vcl/unx/generic/app/i18n_wrp.cxx

On Fri, 2012-02-17 at 14:34 +0000, Michael Meeks wrote:
> On Fri, 2012-02-17 at 14:05 +0000, Caolán McNamara wrote:
> > In fact we should probably go so far as to drop the "generic" vcl plug
> > altogether and just have the gtk and kde ones.
>
> An interesting idea; might be nice to do, though of course, since so
> much code is shared anyway between gtk2, kde and gen - I don't know how
> much we'd actually save (?).

We'd probably save very little from a size perspective, though perhaps
gain a bit from a simplicity perspective. Though on the other hand,
maybe its still convenient to have a "more basic" plugin to refer to
sometimes. Dunno, I'm easy either way.

C.


_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Riccardo Magliocchetti Riccardo Magliocchetti
Reply | Threaded
Open this post in threaded view
|

Re: [REVIEW] Null pointer passed as an argument to a 'nonnull' parameter in vcl/unx/generic/app/i18n_wrp.cxx

Hello,

Il 17/02/2012 15:39, Caolán McNamara ha scritto:

> On Fri, 2012-02-17 at 14:34 +0000, Michael Meeks wrote:
>> On Fri, 2012-02-17 at 14:05 +0000, Caolán McNamara wrote:
>>> In fact we should probably go so far as to drop the "generic" vcl plug
>>> altogether and just have the gtk and kde ones.
>>
>> An interesting idea; might be nice to do, though of course, since so
>> much code is shared anyway between gtk2, kde and gen - I don't know how
>> much we'd actually save (?).
>
> We'd probably save very little from a size perspective, though perhaps
> gain a bit from a simplicity perspective. Though on the other hand,
> maybe its still convenient to have a "more basic" plugin to refer to
> sometimes. Dunno, I'm easy either way.

FYI, I'm actually using these two files to build headless.

     vcl/unx/generic/printer/jobdata \
     vcl/unx/generic/printer/ppdparser \

I don't actually need printing but maybe other interested people do?

thanks

--
Riccardo Magliocchetti
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Tor Lillqvist-2 Tor Lillqvist-2
Reply | Threaded
Open this post in threaded view
|

Re: [REVIEW] Null pointer passed as an argument to a 'nonnull' parameter in vcl/unx/generic/app/i18n_wrp.cxx

In reply to this post by Caolán McNamara
> I doubt it, solaris had moved to a GNOME desktop by default, more or
> less, when I left Sun and last used solaris and all of this is
> irrelevant under the gtk vclplug.

OK, thanks for the technical explanation, if the code in question is
obsolete on any reasonable Solaris platform (whose users might be
interested in LibreOffice in the first place) then I of course have no
objection.

--tml
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Michael Meeks-2 Michael Meeks-2
Reply | Threaded
Open this post in threaded view
|

Re: [REVIEW] Null pointer passed as an argument to a 'nonnull' parameter in vcl/unx/generic/app/i18n_wrp.cxx

In reply to this post by Riccardo Magliocchetti

On Fri, 2012-02-17 at 16:06 +0100, Riccardo Magliocchetti wrote:
> FYI, I'm actually using these two files to build headless.

        Which reminds me - I should review your patch ;-) sorry ...

>      vcl/unx/generic/printer/jobdata \
>      vcl/unx/generic/printer/ppdparser \
>
> I don't actually need printing but maybe other interested people do?

        Ah - so, if these are suitably generic, or can be made so, perhaps we
should whack them into vcl/generic/ instead.  The vcl/unx/generic/ is
rather unix specific, but then some of the generic/ stuff is overly unix
specific too still, but ... :-)

        There's plenty of cleanup / re-factoring & dependency reduction
required in the vcl/ code yet.

        ATB,

                Michael.

--
[hidden email]  <><, Pseudo Engineer, itinerant idiot

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
julien2412 julien2412
Reply | Threaded
Open this post in threaded view
|

[PUSHED] Re: [REVIEW] Null pointer passed as an argument to a 'nonnull' parameter in vcl/unx/generic/app/i18n_wrp.cxx

In reply to this post by julien2412
Hi,

The John Smith's clang report reminded me this thread.
I pushed this easy fix on master (see http://cgit.freedesktop.org/libreoffice/core/commit/?id=4de603d28ce884fb2a3720b6d132465b6dcc98a9).
I don't know if this part of code is or will be used but it can't do any harm.

Julien
John Smith John Smith
Reply | Threaded
Open this post in threaded view
|

Re: [PUSHED] Re: [REVIEW] Null pointer passed as an argument to a 'nonnull' parameter in vcl/unx/generic/app/i18n_wrp.cxx

On Sun, Aug 5, 2012 at 1:55 PM, julien2412 <[hidden email]> wrote:

> Hi,
>
> The John Smith's clang report reminded me this thread.
> I pushed this easy fix on master (see
> http://cgit.freedesktop.org/libreoffice/core/commit/?id=4de603d28ce884fb2a3720b6d132465b6dcc98a9).
> I don't know if this part of code is or will be used but it can't do any
> harm.
>
> Julien
>
That's great! It's nice to see people really taking the time and
effort to look into that clang report.


- John
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice