BUG: fix bug causing misattribution of pnetcdf_var i/o #871
Conversation
shanedsnyder
commented
Dec 8, 2022
shanedsnyder
commented
- various put/get API calls were all using the ncid rather than the varid to retrieve variable records
* various put/get API calls were all using the ncid rather than the varid to retrieve variable records
| PNETCDF_VAR_PRE_RECORD(); | ||
| struct pnetcdf_var_record_ref *rec_ref; | ||
| rec_ref = darshan_lookup_record_ref(pnetcdf_var_runtime->ncid_hash, &ncid, sizeof(int)); | ||
| rec_ref = darshan_lookup_record_ref(pnetcdf_var_runtime->ncid_hash, &varid, sizeof(int)); |
There was a problem hiding this comment.
This looks odd to look up varid in something called ncid_hash. Is this because the ncid_hash data structure is doing double duty to track both depending on whether it is instrumentation for the pnetcdf file module or the pnetcdf var module?
There was a problem hiding this comment.
That's a good point. I was thinking that maybe ncid was a more general thing like a hid is in HDF5 (a hid could reference an open file, dataset, property list, etc.), but looking closer at the PnetCDF interface docs it's not. An ncid (which is just an int) refers specifically to an open file it looks like, so I see how that could get confusing. I'll change the name here to fix that up, too.
There was a problem hiding this comment.
Are both ncids and varids in the same hash, or does each module have its own hash table? At any rate, choosing a more generic name and putting a comment on it would probably help with readability going forward. Otherwise fix looks good.
There was a problem hiding this comment.
They are not in the same hash. Looking closer at the code, I think it was just copying the HDF5 module where the same exact runtime structure could be used for files and datasets since everything is an hid. That works fine with the fix applied previously here, but it is confusing so I did try to fix. Maybe sanity check that 8538154 looks okay?
* `ncid_hash` name was confusing, so just add a new runtime structure for variables that defines a `varid_hash` instead.
carns
left a comment
carns
left a comment
There was a problem hiding this comment.
Good deal, I like this organization with each module having it's own runtime struct with differentiated field names. Should help avoid mixups in the future.
