NOAA-EMC / NCEPLIBS-bufr

The NCEPLIBS-bufr library contains routines and utilites for working with the WMO BUFR format.
Other
44 stars 19 forks source link

question about code in closbf() #393

Closed edwardhartnett closed 1 year ago

edwardhartnett commented 1 year ago

In closebf() we have this code:

      IF ( .NOT. ALLOCATED(NULL) ) THEN
        CALL ERRWRT('++++++++++++++++++++WARNING++++++++++++++++++++++')
        ERRSTR = 'BUFRLIB: CLOSBF WAS CALLED WITHOUT HAVING ' //
     .           'PREVIOUSLY CALLED OPENBF'
        CALL ERRWRT(ERRSTR)
        CALL ERRWRT('++++++++++++++++++++WARNING++++++++++++++++++++++')
        RETURN
      ENDIF

I don't understand how this works (but it does work). When I close a file that I haven't opened, I get this warning.

But what does .NOT.ALLOCATED(NULL) mean?

jbathegit commented 1 year ago

It's possible to output BUFR messages to some place other than a file on the local system. For example, there are applications where output BUFR messages are passed back to the calling program in a memory array (and then the user can do whatever they want with them), vs. the library automatically doing an internal write to a file. Feel free take a look at the documentation for writsa and scroll down to the Remarks for more details if you like.

At any rate, in these particular cases a Fortran logical unit number still needs to be assigned to the associated file stream, even though it's not going to be connected to any actual file on the local system. And that's where the NULL array comes in - it's a dynamically-allocated array that's set within openbf for any such Fortran logical units, so that way when closbf comes along later there's a way to know whether there's actually a system file connected to that unit number. And if not, then there's no need to actually "close" that unit number.

So that's what's going on with the NULL array, but as for why there's even a safety hatch in the first place for calling closbf without having ever called openbf, that goes way back to PR #10 (which in turn links to issue #9) where Mark P., Rahul and others pleaded with me to add this to the baseline b/c the GSI apparently had some haywire loop where closbf might get called for a particular unit before openbf was ever called, and in such cases you'd end up with a segfault because the NULL array had never been allocated. And while my first obvious response at that time was "Well, then they should fix the GSI code to stop doing that!", that was apparently a more painful solution, and so in the end I relented and agreed to add this somewhat-innocuous failsafe check into closbf.

(Aren't you glad you asked? ;-)

edwardhartnett commented 1 year ago

OK, after rereading that reply, I think I understand. We actually have an internal array called "NULL". Now that makes a lot more sense.

I would submit that if any of our user-facing functions segfault for any input, that's a serious bug in our library. We need to check input sufficiently so that no segfaults occur. When they do, we need to add a test to reproduce the problem, then we need to fix it (and retain the test, so that it never gets broken again).

So I think this idea of checking whether NULL has been allocated is a good one, and something similar should be done in status() to resolve #395. status() is not a public facing function but it is called by a lot of public facing functions with whatever dumb parameter values the user passed in. So we need to handle those dumb values. ;-)