fix nc_open_mem

[thehesiod]

• fixes nc_open_mem fails #394
• requires: Fix NC_DISKLESS returns garbage data for certain files #403

[thehesiod]

for those who are on OSX I've created a custom brew tap with the patch: https://github.com/thehesiod/homebrew-tap/blob/master/Formula/netcdf.rb

[thehesiod]

btw, is there a way to create a diskless netCDF file, then get the bytes back w/o touching the disk? Something like nc_sync_and_return_bytes. Would be really useful for testing

[DennisHeimbigner]

Not at the moment. And I am not sure it is doable for netcdf-4 because we rely on
the HDF5 in memory capability for netcdf-4.

[DennisHeimbigner]

After a quick look, it looks like we would
have to write a custom driver; probably a wrapper around
the existing H5FD_CORE driver would work. But sufficiently
challenging so that we are not likely to tackle this any time soon.

[thehesiod]

thanks for checking! Hopefully this patch can land soon.

[thehesiod]

ack, even with this fix it seems like for non trivial files it's returning corrupt data. With the following file:
foo.zip

and the following code:

#include <iostream>
#include <netcdf.h>
#include <netcdf_mem.h>
#include <iostream>
#include <fstream>
#include <assert.h>
#import <vector>


using namespace std;

#define    HRAPY  200
#define    HRAPX  333

static size_t value_count[] = {HRAPY, HRAPX};
static size_t start[] = {0, 0};

int main(int argc, const char * argv[]) {
    ifstream file("/tmp/foo.nc", ios::in | ios::binary | ios::ate);
    streamsize size = file.tellg();
    file.seekg(0, ios::beg);
    
    std::vector<char> buffer(size);
    int status;
    int ncid;
    
    if (file.read(buffer.data(), size))
    {
        status = nc_open_mem(".nc", 0, size, buffer.data(), &ncid);
        if(status != NC_NOERR) {
            cout << nc_strerror(status);
            assert(false);
        }
        
        int rh_id;
        status = nc_inq_varid(ncid, "amountofprecip", &rh_id);
        if(status != NC_NOERR) {
            cout << nc_strerror(status);
            assert(false);
        }
        
        int rh_vals[HRAPY][HRAPX];
        status = nc_get_vara_int(ncid, rh_id, start, value_count, (int*)rh_vals);
        if(status != NC_NOERR) {
            cout << nc_strerror(status);
            assert(false);
        }
    }

    return 0;
}

note there's no errors, however rows > 100 return garbage, row 100 looks like:

[ -71   76   96   37   20    8  -67  -47  -89   62   52  -52   78   86  -62
   60   46   44  -68  -44  -34   28   30  -30   -2   -1   14  -64  -68  -63
  -88  -86  -95   40  -70  -55    5  -56  -50  -61   88   57   45  -73   -3
  -45   19    5  -73   -6  -

whereas row 101 looks like:

[ 4325376  2818048   393215 -3997697 -4063233 -1114113 -1310720  4915199
 -6488064  5832703 -1114113 -2228225 -2686977 -3932160  6160384  3276799
  -720896  3670015 -4063232  6225919 -5636096  1966080  3014655 -4063233
 -5308416  1114112  3407871 -4653057 

opening the same file with nc_open does not return corrupt data.

[thehesiod]

looks like it returns garbage after first iteration of for loop in ncx_getn_int_int

[thehesiod]

just logged #401 as I found this happens even with NC_DISKLESS mode.

[thehesiod]

ok, have a fix for NC_DISKLESS/nc_open_mem, not sure why it works but so far it seems to work ok. Let me know what you guys think.

[WardF]

@DennisHeimbigner This looks straightforward to me but since it is your code I thought I'd wait for you to weigh in before doing anything. It's a very small change and at a glance appears to be a good one.

[DennisHeimbigner]

Ok,digging deep,what I am seeing is a swap failure because
of a two byte discrepency in addresses. This is so weird that
I don't entirely believe it.

[DennisHeimbigner]

Pull request #403 must be applied before applying this pull request

[thehesiod]

ok, reverted my hack, @DennisHeimbigner could you comment on the comment

/* Use half the filesize as the blocksize ; why? */

here: https://github.com/Unidata/netcdf-c/blob/master/libsrc/memio.c#L385
perhaps this is not needed anymore?

[thehesiod]

@WardF If you wouldn't mind, I think it would be good adding a test for memio so it doesn't break again.

[WardF]

No problem, but it appears that Dennis added a test as part of his upull request

[thehesiod]

@WardF confused because if he added a test it should fail w/o my fix. Ya I see the test supports opening a file from mem but I think it turns off the mem support by default (undef MEM). Pinging @DennisHeimbigner I'm guessing he added a test that theoretically could be run as DISKLESS or MEM but right now is DISKLESS.

[DennisHeimbigner]

I added a modified version of your code as our testcase; assume you don't mind
my using your code.

[thehesiod]

@DennisHeimbigner np, does that mean your PR fixes the issue this PR attempts to fix, or your testcase is currently failing in your PR?

[DennisHeimbigner]

I'm guessing he added a test that theoretically could be run as DISKLESS or MEM
but right now is DISKLESS.
Correct, it turns out that using nc_open_mem is irrelevant to the problem. The problem
was with reading a file using NC_DISKLESS. I had that define in the test case as a holdover
from when I was trying to determine the cause of this problem. Shall I remove it?

[thehesiod]

@DennisHeimbigner sorry, there were two issues, let me explain:

Issue 1 (nc_open_mem returns error code):
At first I was unable to use nc_open_mem because it returns OSError, so I created this PR (note setting diskless flag), that got rid of the OSError.

Issue 2 (corrupt data):
After the fix from this PR, I noticed that the data was corrupt, and created that second part about DISKLESS mode (which your PR fixes).

So we're wondering if you also want to fix what this fixes in your PR, which would require 2 testcases, or perhaps one uber testcase of nc_open_mem w/ CDF5 data.

[DennisHeimbigner]

No, we should probably have a separate test case for the issue 1 (nc_open_mem returns error).
the testcase I added is intended soley to address issue 2 (garbage data). I assumed your issue 1
pr had a test case. Is that not so?

[DennisHeimbigner]

Ok, another problem. When I run my test case using nc_open_mem, it works fine
and the bug fixed here is not occuring?
Ward, were you able to duplicate this bug (nc_open_mem failure)?

[WardF]

I will check in a bit; I was focused on diskless before, and not open mem. I'm in meetings currently and for much of the rest of the day but will attempt to duplicate ASAP.

[DennisHeimbigner]

Alex- can you post the dfile.c that you are using (with or without this pr)?

[thehesiod]

ya, working on it now, still fighting MADIS data :)

[thehesiod]

ok, just used the latest 4.4.1.1 release with the following code:

#include <iostream>
#include <netcdf.h>
#include <netcdf_mem.h>
#include <iostream>
#include <fstream>
#include <assert.h>
#import <vector>


using namespace std;


int main(int argc, const char * argv[]) {
    ifstream file("/tmp/20160513_1700.nc", ios::in | ios::binary | ios::ate);
    streamsize size = file.tellg();
    file.seekg(0, ios::beg);
    
    std::vector<char> buffer(size);
    int status;
    int ncid;
    
    if (file.read(buffer.data(), size))
    {
        status = nc_open_mem(".nc", 0, size, buffer.data(), &ncid);
        if(status != NC_NOERR) {
            cout << nc_strerror(status);
            assert(false);
        }
        
        std::cout << "Success!\n";
    }
    
    return 0;
}

and this file: https://madis-data.bldr.ncep.noaa.gov/madisPublic1/data/archive/2016/05/13/LDAD/mesonet/netCDF/20160513_1700.gz

and I get No such file or directory

[thehesiod]

regarding difile.c, can get it from my branch: https://github.com/thehesiod/netcdf-c/blob/nc_open_mem_fix/libdispatch/dfile.c

[DennisHeimbigner]

Sorry, I just cannot duplcate the failure.

[thehesiod]

ok, let me try on debian

[thehesiod]

ok my steps to repro:

docker run --rm -ti debian
apt-get update
apt-get install -y wget clang build-essential libcurl4-gnutls-dev libhdf4-dev libhdf5-dev vim
cd /tmp

# get test file
wget https://madis-data.bldr.ncep.noaa.gov/madisPublic1/data/archive/2016/05/13/LDAD/mesonet/netCDF/20160513_1700.gz
gunzip 20160513_1700.gz
mv 20160513_1700 20160513_1700.nc

# install cmake
wget https://cmake.org/files/v3.8/cmake-3.8.0-Linux-x86_64.sh && \
    chmod u+x cmake-3.8.0-Linux-x86_64.sh && \
    ./cmake-3.8.0-Linux-x86_64.sh --prefix=/usr/local --skip-license

# build netcdf-c
wget https://github.com/Unidata/netcdf-c/archive/v4.4.1.1.tar.gz && \
    tar xvpf v4.4.1.1.tar.gz

cd netcdf-c-4.4.1.1
mkdir build_dir
cd build_dir
cmake .. -DCMAKE_BUILD_TYPE=RELEASE -DHAVE_HDF5_H=/usr/include/hdf5/serial/hdf5.h
make -j$(nproc) install

# build test app
cd /tmp
clang++ test.cpp -lnetcdf -o test
./test

results in:

root@52c457f5b5f5:/tmp# ./test 
test: test.cpp:27: int main(int, const char **): Assertion `false' failed.
No such file or directoryAborted

[WardF]

Thanks for providing a docker-based solution to reproduce this! I'll follow up on this.

[WardF]

Looks good.