From 2a4dc88f8a9c27634ecb7b0219552d640f4af68a Mon Sep 17 00:00:00 2001 From: Tim Jenness Date: Fri, 24 Oct 2014 11:10:23 -0700 Subject: [PATCH] hds: Change the way fortran locators work Locators in Fortran are (oddly) string buffers and not simple integers. Previously the contents of "struct LOC" was copied into the buffer on "export" and copied from the buffer on "import". This worked okay except that the size of the struct governed the value of DAT__SZLOC. Any changes meant a full recompile. It also seems excessive to risk have each locator use tens of bytes of string buffer when only a few bytes are really needed. This current implementation stores the pointer in the Fortran buffer and reads the pointer out of the buffer on import. It's not very efficient as it currently really does store a formatted version of the pointer (via sprintf %p) and uses strtol() to convert that number back to a pointer. It does work though and DAT__SZLOC is now invariant to the structure size. This will be important for HDS v5. Obviously at some point we should just store the 8 bytes into the buffer and cast the 8 bytes back to a pointer again but first having to check that the string locators () still work. Note that this patch adds a dependency to ONE. This may just be temporary. --- libraries/hds/Makefile.am | 2 +- libraries/hds/dat1.h | 4 +- libraries/hds/dat1_import_floc.c | 81 +++++++++++++++++++++----------- libraries/hds/datExportFloc.c | 73 +++++++++++++++++++--------- libraries/hds/datImportFloc.c | 77 ++++++++++++++++-------------- 5 files changed, 146 insertions(+), 91 deletions(-) diff --git a/libraries/hds/Makefile.am b/libraries/hds/Makefile.am index a542b2e4152..7a7f4407734 100644 --- a/libraries/hds/Makefile.am +++ b/libraries/hds/Makefile.am @@ -25,7 +25,7 @@ libhdsf_la_SOURCES = \ $(F_C_ROUTINES) \ $(BLOCK_DATA_FILES) -libhdsf_la_LIBADD = `ems_link` +libhdsf_la_LIBADD = `ems_link` `one_link` # Make all library code position independent. This is handy for creating # shareable libraries from the static ones (Java JNI libraries). diff --git a/libraries/hds/dat1.h b/libraries/hds/dat1.h index 339576ca156..048626790d7 100644 --- a/libraries/hds/dat1.h +++ b/libraries/hds/dat1.h @@ -407,8 +407,6 @@ if (!_ok(*status))\ int hds1_get_subs( int ndim, HDS_PTYPE *dims, INT_BIG offset, HDS_PTYPE *subs ); - int dat1_import_floc ( const char flocator[DAT__SZLOC], - int loc_length, HDSLoc *clocator, - int * status); + HDSLoc * dat1_import_floc ( const char flocator[DAT__SZLOC], int loc_length, int * status); #endif diff --git a/libraries/hds/dat1_import_floc.c b/libraries/hds/dat1_import_floc.c index 9e062550e83..0513e112a95 100644 --- a/libraries/hds/dat1_import_floc.c +++ b/libraries/hds/dat1_import_floc.c @@ -23,12 +23,11 @@ * Import a fortran HDS locator buffer into C * Invocation: - * dat1_import_floc( const char flocator[DAT__SZLOC], int len, HDSLoc *clocator, int * status); + * clocator = dat1_import_floc( const char flocator[DAT__SZLOC], int len, int * status); * Description: * This function should be used to convert a Fortran HDS locator - * (implemented as a string buffer) to a C locator struct. It is - * for internal usage by HDS only. The public version is datImportFloc. + * (implemented as a string buffer) to a C locator struct. * Arguments * flocator = const char * (Given) @@ -36,15 +35,14 @@ * characters long. * len = int (Given) * Size of Fortran character buffer. Sanity check. - * clocator = HDSLoc * (Returned) - * Fills the supplied HDSLoc struct with the contents of the fortran buffer. - * The C struct will not be malloced by this routine. * status = int * (Given and Returned) * Inherited status. Attempts to execute even if status is not DAT__OK * on entry. - * Returned: - * int status = Inherited status value + * Returned Value: + * clocator = HDSLoc * + * C HDS locator corresponding to the Fortran locator. + * Should be freed by using datAnnul(). * Authors: * Tim Jenness (JAC, Hawaii) @@ -55,17 +53,25 @@ * Initial version * 27-JAN-2006 (DSB) * Attempt to execute even if status is set on entry. + * 2014-09-07 (TIMJ): + * Complete rewrite to now extract pointer value from buffer. API change + * to return the pointer directly. * Notes: - * Does not check the contents of the locator for validity but does check for - * common Fortran error locators such as DAT__ROOT and DAT__NOLOC. + * - Does not check the contents of the locator for validity but does check for + * common Fortran error locators such as DAT__ROOT and DAT__NOLOC. + * - For internal usage by HDS only. The public version is datImportFloc. + * - Differs from the original HDS API in that it returns the locator. + * - Attempts to execute even if status is bad. + * - This change has been made to allow the struct to be extended without forcing + * a recompile when DAT__SZLOC changes. * See Also: * - datImportFloc * - datExportFloc - * - dat1_free_hdsloc * Copyright: + * Copyright (C) 2014 Cornell University * Copyright (C) 2005 Particle Physics and Astronomy Research Council. * All Rights Reserved. @@ -91,43 +97,62 @@ *- */ -int dat1_import_floc ( const char flocator[DAT__SZLOC], int loc_length, HDSLoc * clocator, int * status) { +HDSLoc * +dat1_import_floc ( const char flocator[DAT__SZLOC], int loc_length, int * status) { -/* Validate the locator length. */ + long ptr_as_long = 0; + HDSLoc * clocator = NULL; + + /* Validate the locator length. */ if (loc_length != DAT__SZLOC ) { if (*status == DAT__OK ) { *status = DAT__LOCIN; - emsSeti( "LEN", loc_length ); - emsSeti( "SZLOC", DAT__SZLOC ); - emsRep( "DAT1_IMPORT_FLOC", "Locator length is ^LEN not ^SZLOC", status); + emsRepf( "DAT1_IMPORT_FLOC", "Locator length is %d not %d", status, + loc_length, DAT__SZLOC); } - return *status; + return NULL; }; /* Check obvious error conditions */ if (strncmp( DAT__ROOT, flocator, loc_length) == 0 ){ if( *status == DAT__OK ) { *status = DAT__LOCIN; - emsRep( "datImportFloc_ROOT", "Input HDS Locator corresponds to DAT__ROOT but that can only be used from NDF", status ); + emsRep( "dat1ImportFloc_ROOT", + "Input HDS Locator corresponds to DAT__ROOT but that can only be used from NDF", + status ); } - return *status; + return NULL; } /* Check obvious error conditions */ if (strncmp( DAT__NOLOC, flocator, loc_length) == 0 ){ if( *status == DAT__OK ) { *status = DAT__LOCIN; - emsRep( "datImportFloc_NOLOC", "Input HDS Locator corresponds to DAT__NOLOC but status is good (Possible programming error)", status ); + emsRep( "datImportFloc_NOLOC", + "Input HDS Locator corresponds to DAT__NOLOC but status is good (Possible programming error)", + status ); } - return *status; + return NULL; } -/* If OK, then extract the information from the locator string (necessary */ -/* to ensure that data alignment is correct, as the string will normally be */ -/* stored externally in a Fortran CHARACTER variable). */ -/* We do this copy regardless of status since this is sometimes required - and it can't hurt if we have allocated memory */ - memmove( clocator, flocator, sizeof( struct LOC ) ); + /* Everything seems to be okay so now convert the string buffer to the + required pointer. We ignore status as sometimes we need to try + to get the value regardless (otherwise DAT_ANNUL from Fortran would + never succeed). */ + + ptr_as_long = strtol( flocator, NULL, 16 ); + + if (ptr_as_long == 0) { + /* This should not have happened */ + if (*status == DAT__OK) { + *status = DAT__LOCIN; + emsRep("dat1_import_floc_3", + "Error importing locator from Fortran", status ); + return NULL; + } + } - return *status; + /* Do the cast */ + clocator = (HDSLoc *)ptr_as_long; + return clocator; } diff --git a/libraries/hds/datExportFloc.c b/libraries/hds/datExportFloc.c index cfd2f1bfba6..94f459d35db 100644 --- a/libraries/hds/datExportFloc.c +++ b/libraries/hds/datExportFloc.c @@ -7,6 +7,7 @@ #include #include "ems.h" +#include "star/one.h" #include "hds1.h" #include "rec.h" @@ -27,6 +28,9 @@ * Language: * Starlink ANSI C + * Type of Module: + * Library routine + * Invocation: * datExportFloc( HDSLoc** clocator, int free, int len, char flocator[DAT__SZLOC], int * status ); @@ -37,26 +41,30 @@ * HDS_EXPORT_CLOCATOR macro defined in hds_fortran.h. * Arguments: - * HDSLoc ** clocator = Given & Returned - * Pointer to Locator to be exported. If the memory is freed, the - * locator will be set to NULL on return, else it will be untouched. - * int free = Given - * If true (1) the C locator is freed once the + * clocator = HDSLoc ** (Given and Returned) + * Pointer to the locator to be exported. See the "free" description below + * as to whether this locator will be annulled or not. + * free = int (Given) + * If true (1) the C locator is nulled out once the * Fortran locator is populated. If false, the locator memory is not - * touched. - * int len = Given - * Size of Fortran character buffer to recieve the locator. Sanity check. - * char flocator[DAT__SZLOC] = Returned + * touched. Regardless, the locator itself is not annulled as it is + * now simply referenced from the Fortran. + * len = int (Given) + * Size of Fortran character buffer to receive the locator. Sanity check. + * Should be DAT__SZLOC. + * flocator = char [DAT__SZLOC] (Returned) * Fortran character string buffer. Should be at least DAT__SZLOC * characters long. If clocator is NULL, fills the buffer * with DAT__NOLOC. - * int * status = Given & Returned + * status = int* (Given and Returned) * Inherited status. If status is bad the Fortran locator will be * filled with DAT__NOLOC. The memory associated with clocator will * be freed if free is true regardless of status. * Authors: * TIMJ: Tim Jenness (JAC, Hawaii) + * TIMJ: Tim Jenness (Cornell) + * {enter_new_authors_here} * History: * 16-NOV-2005 (TIMJ): @@ -64,6 +72,10 @@ * 18-NOV-2005 (TIMJ): * Make semi public to allow other Fortran wrappers to use this function. * Rename from dat1_export_floc to datExportFloc + * 2014-09-07 (TIMJ): + * Rewrite to store the pointer (as string) directly in the Fortran + * string buffer rather than the contents of the struct. + * {enter_further_changes_here} * Notes: * - Fortran locator string must be preallocted. The C locator can be freed @@ -73,11 +85,17 @@ * wrapping Fortran layers from C. "Export" means to export a native * C locator to Fortran. * - There is no Fortran eqiuvalent to this routine. + * - This routine differs from the previous HDS implementation in that the address + * of the supplied pointer is stored in the Fortran string buffer and not the contents + * of the struct. This is done to constrain the required size of the locator + * in Fortran to allow the locator structure to change without forcing a full recompile + * of everything. * See Also: * datImportFloc * Copyright: + * Copyright (C) 2014 Cornell University * Copyright (C) 2005 Particle Physics and Astronomy Research Council. * All Rights Reserved. @@ -105,27 +123,36 @@ void datExportFloc ( HDSLoc **clocator, int free, int loc_length, char flocator[DAT__SZLOC], int * status) { -/* Validate the locator length. */ + /* Validate the locator length */ if (*status == DAT__OK && loc_length != DAT__SZLOC ) { *status = DAT__LOCIN; - emsSeti( "LEN", loc_length ); - emsSeti( "SZLOC", DAT__SZLOC ); - emsRep( "datExportFloc", "Locator length is ^LEN not ^SZLOC", status); - }; - -/* If OK, then extract the information from the locator string (necessary */ -/* to ensure that data alignment is correct, as the string will normally be */ -/* stored externally in a Fortran CHARACTER variable). */ + emsRepf( "datExportFloc", "Locator length is %d not %d", status, + loc_length, DAT__SZLOC); + } + /* if everything is okay we store the pointer location in the Fortran + locator */ if ( *status == DAT__OK && *clocator != NULL ) { - memmove( flocator, *clocator, sizeof( struct LOC ) ); + + /* We export from C by storing the pointer of the C struct in the + Fortran character buffer. We can not store a clone in the Fortran + locator because clones are documented to not clone mapped status + and DAT_MAP / DAT_UNMAP will fail for clones. We just store the + supplied locator and null out the C version if we are being requested + to free it. Note that if we free=false the caller should not then + annul the locator as that would mess up the Fortran side. If the current + scheme does not work, we could try assigning the clone to the caller and + the original to the fortran locator but this requires some thought. */ + + one_snprintf(flocator, loc_length, "%p", status, *clocator ); + } else { strncpy( flocator, DAT__NOLOC, DAT__SZLOC ); } - - /* Free regardless of status */ - if (free) dat1_free_hdsloc( clocator ); + /* Null out the caller if requested. Do not annul as we have stored + the original pointer in the Fortran layer */ + if (free) *clocator = NULL; return; } diff --git a/libraries/hds/datImportFloc.c b/libraries/hds/datImportFloc.c index 8ba868f81a2..e77a22bac77 100644 --- a/libraries/hds/datImportFloc.c +++ b/libraries/hds/datImportFloc.c @@ -7,7 +7,6 @@ #include #include "ems.h" -#include "star/mem.h" #include "hds1.h" #include "rec.h" @@ -23,37 +22,37 @@ * datImportFloc * Purpose: - * Import a Fortran HDS locator buffer into C with malloc + * Import a Fortran HDS locator buffer into C * Invocation: * datImportFloc( const char flocator[DAT__SZLOC], int loc_length, HDSLoc **clocator, int * status); * Description: * This function should be used to convert a Fortran HDS locator - * (implemented as a string buffer) to a C locator struct. The C locator - * is malloced by this routine. The memory will be freed when datAnnul - * is called. This function is also available via the + * (implemented as a string buffer) to a C locator struct. + * This function is also available via the * HDS_IMPORT_FLOCATOR macro defined in hds_fortran.h. * Arguments - * const char flocator[DAT__SZLOC] = Given + * flocator = const char [DAT__SZLOC] (Given) * Fortran character string buffer. Should be at least DAT__SZLOC * characters long. - * int loc_length = Given + * loc_length = int (Given) * Size of Fortran character buffer. Sanity check. - * HDSLoc ** clocator = Returned - * Fills the HDSLoc struct with the contents of the fortran buffer. - * Locator struct is malloced by this routine and should be freed - * either with dat1_free_hdsloc or dat1_export_loc. *clocator must - * be NULL on entry. If status is set by this routine, the struct - * will not be malloced on return. - * int *status = Given and Returned + * Should be DAT__SZLOC. + * clocator = HDSLoc ** (Returned) + * Assigned to the C version of the Fortran locator. + * Must be NULL on entry. Memory is not allocated by this routine + * and the locator must not be annulled whilst the Fortran + * locator is still being used. + * status = int * (Given and Returned) * Inherited status. Attempts to excecute even if status is not SAI__OK * on entry. * Authors: * Tim Jenness (JAC, Hawaii) * David Berry (JAC, Preston) + * Tim Jenness (Cornell) * History: * 16-NOV-2005 (TIMJ): @@ -73,19 +72,27 @@ * 22-OCT-2010 (DSB): * Return a NULL pointer without error if the supplied pointer is * DAT__NOLOC. + * 2014-09-07 (TIMJ): + * Complete rewrite. No long allocate and memcpy, just extract the + * pointer directly from the buffer. * Notes: - * - A NULL pointer is returne dif the supplied locator is DAT__NOLOC. - * - Does not check the contents of the locator for validity. + * - A NULL pointer is returned if the supplied locator is DAT__NOLOC. + * - Does check the contents of the locator for validity to avoid + * uncertainty when receiving uninitialized memory. * - The expectation is that this routine is used solely for C * interfaces to Fortran library routines. + * - The locator returned by this routine has not been allocated + * independently. It is the same locator tracked by the Fortran + * layer. Do not annul this locator if the Fortran layer is still + * tracking it. * See Also: * - datExportFloc * Copyright: -* Copyright (C) 2010 Science & Technology Facilities Council. -* All Rights Reserved. + * Copyright (C) 2014 Cornell University. + * Copyright (C) 2010 Science & Technology Facilities Council. * Copyright (C) 2005-2006 Particle Physics and Astronomy Research Council. * All Rights Reserved. @@ -124,22 +131,22 @@ void datImportFloc ( const char flocator[DAT__SZLOC], int loc_length, HDSLoc ** return; } -/* Return the NULL pointer unchanged if the supplied locator is DAT__NOLOC. */ - if( strncmp( DAT__NOLOC, flocator, loc_length ) ) { - - /* See if we need to allocate memory for the locator struct */ - /* Allocate some memory to hold the C structure */ - - *clocator = MEM_MALLOC( sizeof( struct LOC ) ); + /* For these HDS/HDF5 locators that are either hex pointers or "" strings + we can validate them immediately */ + if (flocator[0] != '0' && flocator[0] != '<') { + if (*status == DAT__OK) { + char flocstr[DAT__SZLOC+1]; + *status = DAT__WEIRD; + memmove( flocstr, flocator, DAT__SZLOC); + emsRepf( "datImportFloc_2", + "datImportFloc: Supplied Fortran locator looks to be corrupt: '%s'", + status, flocstr ); + } + return; + } - if (*clocator == NULL ) { - if( *status == DAT__OK ) { - *status = DAT__NOMEM; - emsRep( "datImportFloc", "datImportFloc: No memory for C locator struct", - status); - } - return; - } + /* Return the NULL pointer unchanged if the supplied locator is DAT__NOLOC. */ + if( strncmp( DAT__NOLOC, flocator, loc_length ) ) { /* Now import the Fortran locator - this will work even if status is bad on entry but it is possible for this routine to set status @@ -148,10 +155,8 @@ void datImportFloc ( const char flocator[DAT__SZLOC], int loc_length, HDSLoc ** HDS locator. */ emsMark(); lstat = DAT__OK; - dat1_import_floc( flocator, loc_length, *clocator, &lstat); + *clocator = dat1_import_floc( flocator, loc_length, &lstat); if (lstat != DAT__OK) { - /* free the memory and trigger a NULL pointer */ - dat1_free_hdsloc( clocator ); /* Annul all this if status was already bad, since we do not want the extra meaningless messages on the stack. If status