Check datatype match inside segy_set_field

.datatype field is kind of pointless at the moment.
- In C we can't easily force someone to set datatype the same moment
when .value is set. So nothing prevents us from setting value to i16 and
datatype to u32.
- In theory we can simply pass segy_field_value as an argument instead
of passing full segy_field_data. However it doesn't fit into the spirit
of function, as without knowing .datatype we can't interpret value on
its own and are forced to do it according to the field mapping table
instead of the actual datatype.

The best option seems to force .datatype being set by checking that it
corresponds to the datatype retrieved from field mapping.

Additionally fd doesn't have to be a pointer. We should treat that value
as any other local primitive as it is not that big in size.

Author: achaikou

lib/include/segyio/segy.h

@@ -198,7 +198,7 @@ int segy_set_endianness( segy_datasource*, int opt );
 int segy_field_datatype( int field );
 
 int segy_get_field( const char* header, int field, segy_field_data* fd );
-int segy_set_field( char* header, int field, segy_field_data* fd );
+int segy_set_field( char* header, int field, segy_field_data fd );
 
 int segy_get_field_int( const char* header, int field, int* f );
 int segy_set_field_int( char* header, const int field, const int val );

lib/src/segy.c

@@ -920,59 +920,65 @@ int segy_get_field_int( const char* header, int field, int* val ) {
     return fd_get_int( &fd, val );
 }
 
-int segy_set_field( char* header, int field, segy_field_data* fd ) {
+int segy_set_field( char* header, int field, segy_field_data fd ) {
     int offset = field - 1;
     if (offset >= SEGY_TEXT_HEADER_SIZE) {
         offset -= SEGY_TEXT_HEADER_SIZE;
     }
 
-    segy_field_value fv = fd->value;
+    segy_field_value fv = fd.value;
     uint64_t val;
-    switch ( fd->datatype ) {
+
+    uint8_t datatype = segy_field_datatype( field );
+    if (fd.datatype != datatype) {
+        return SEGY_INVALID_FIELD_DATATYPE;
+    }
+
+    switch ( fd.datatype ) {
 
         case SEGY_SIGNED_INTEGER_8_BYTE:
             fv.i64 = htobe64( fv.i64 );
-            memcpy( header + offset, &(fv.i64), formatsize( fd->datatype ));
+            memcpy( header + offset, &(fv.i64), formatsize( fd.datatype ));
             return SEGY_OK;
 
         case SEGY_SIGNED_INTEGER_4_BYTE:
             fv.i32 = htobe32( fv.i32 );
-            memcpy( header + offset, &(fv.i32), formatsize( fd->datatype ));
+            memcpy( header + offset, &(fv.i32), formatsize( fd.datatype ));
             return SEGY_OK;
 
         case SEGY_SIGNED_SHORT_2_BYTE:
             fv.i16 = htobe16( fv.i16 );
-            memcpy( header + offset, &(fv.i16), formatsize( fd->datatype ));
+            memcpy( header + offset, &(fv.i16), formatsize( fd.datatype ));
             return SEGY_OK;
 
         case SEGY_SIGNED_CHAR_1_BYTE:
-            memcpy( header + offset, &(fv.i8), formatsize( fd->datatype ));
+            memcpy( header + offset, &(fv.i8), formatsize( fd.datatype ));
             return SEGY_OK;
 
         case SEGY_UNSIGNED_INTEGER_8_BYTE:
             fv.u64 = htobe64( fv.u64 );
-            memcpy( header + offset, &(fv.u64), formatsize( fd->datatype ));
+            memcpy( header + offset, &(fv.u64), formatsize( fd.datatype ));
             return SEGY_OK;
 
         case SEGY_UNSIGNED_INTEGER_4_BYTE:
             fv.u32 = htobe32( fv.u32 );
-            memcpy( header + offset, &(fv.u32), formatsize( fd->datatype ));
+            memcpy( header + offset, &(fv.u32), formatsize( fd.datatype ));
             return SEGY_OK;
 
         case SEGY_UNSIGNED_SHORT_2_BYTE:
             fv.u16 = htobe16( fv.u16 );
-            memcpy( header + offset, &(fv.u16), formatsize( fd->datatype ));
+            memcpy( header + offset, &(fv.u16), formatsize( fd.datatype ));
             return SEGY_OK;
 
         case SEGY_UNSIGNED_CHAR_1_BYTE:
-            memcpy( header + offset, &(fv.u8), formatsize( fd->datatype ));
+            memcpy( header + offset, &(fv.u8), formatsize( fd.datatype ));
             return SEGY_OK;
 
         case SEGY_IEEE_FLOAT_8_BYTE:
             memcpy( &val, &(fv.f64), sizeof( uint64_t ) );
             val = htobe64( val );
             memcpy( &(fv.f64), &val, sizeof( uint64_t ) );
-            memcpy( header + offset, &(fv.f64), formatsize( fd->datatype ));
+            memcpy( header + offset, &(fv.f64), formatsize( fd.datatype ));
             return SEGY_OK;
 
         default:
@@ -1023,7 +1029,7 @@ int segy_set_field_int( char* header, const int field, const int val ) {
     int err = fd_set_int( &fd, val );
     if( err != SEGY_OK ) return err;
 
-    return segy_set_field( header, field, &fd );
+    return segy_set_field( header, field, fd );
 }
 
 static int slicelength( int start, int stop, int step ) {

lib/src/segy.def

@@ -12,6 +12,8 @@ segy_sample_interval
 segy_format
 segy_set_format
 segy_set_endianness
+segy_get_field
+segy_set_field
 segy_get_field_int
 segy_set_field_int
 segy_field_forall

lib/test/segy.cpp

@@ -69,10 +69,11 @@ struct Err {
     bool operator == ( Err other ) const { return this->err == other.err; }
     bool operator != ( Err other ) const { return !(*this == other); }
 
-    static Err ok()    { return SEGY_OK; }
-    static Err args()  { return SEGY_INVALID_ARGS; }
-    static Err field() { return SEGY_INVALID_FIELD; }
-    static Err value() { return SEGY_INVALID_FIELD_VALUE; }
+    static Err ok()       { return SEGY_OK; }
+    static Err args()     { return SEGY_INVALID_ARGS; }
+    static Err field()    { return SEGY_INVALID_FIELD; }
+    static Err value()    { return SEGY_INVALID_FIELD_VALUE; }
+    static Err datatype() { return SEGY_INVALID_FIELD_DATATYPE; }
 
     int err;
 };
@@ -2053,10 +2054,11 @@ TEST_CASE("segy_get_field reads values correctly",  "[c.segy]" ) {
         header[SEGY_TR_TRACE_ID-1] = b1;
         header[SEGY_TR_TRACE_ID-0] = b0;
 
-        int read_value;
-        Err err = segy_get_field_int( header, SEGY_TR_TRACE_ID, &read_value );
+        segy_field_data read_value;
+        Err err = segy_get_field( header, SEGY_TR_TRACE_ID, &read_value );
         CHECK( success( err ) );
-        CHECK( read_value == value );
+        CHECK( read_value.datatype == SEGY_SIGNED_SHORT_2_BYTE );
+        CHECK( read_value.value.i16 == value );
     }
 }
 
@@ -2066,7 +2068,11 @@ TEST_CASE("segy_set_field write values correctly",  "[c.segy]" ) {
 
     SECTION("test edge cases int16") {
         int16_t value = GENERATE(0, 1, -1, 0x0102, 0x0201, -32767, -32766);
-        Err err = segy_set_field_int( header, SEGY_TR_TRACE_ID, value );
+
+        segy_field_data fd;
+        fd.datatype = SEGY_SIGNED_SHORT_2_BYTE;
+        fd.value.i16 = value;
+        Err err = segy_set_field( header, SEGY_TR_TRACE_ID, fd );
         CHECK( err == Err::ok() );
 
         uint8_t b0 = header[SEGY_TR_TRACE_ID-0];
@@ -2080,9 +2086,17 @@ TEST_CASE("segy_set_field write invalid value",  "[c.segy]" ) {
 
     char header[ SEGY_TRACE_HEADER_SIZE ] = { 0 };
 
-    SECTION("test value outside of int16 range") {
+    SECTION("test value outside of int16 range for segy_set_field_int") {
         int32_t value = 0xFFFF + 1;
         Err err = segy_set_field_int( header, SEGY_TR_TRACE_ID, value );
         CHECK( err == Err::value() );
     }
+
+     SECTION("test mismatched type") {
+        segy_field_data fd;
+        fd.value.i16 = 42;
+        fd.datatype = SEGY_SIGNED_INTEGER_4_BYTE;
+        Err err = segy_set_field( header, SEGY_TR_TRACE_ID, fd );
+        CHECK( err == Err::datatype() );
+    }
 }

python/segyio/segyio.cpp

@@ -1846,7 +1846,7 @@ PyObject* putfield( PyObject*, PyObject *args ) {
             return KeyError( "Field %d has unknown datatype %d", field, fd.datatype );
     }
 
-    int err = segy_set_field( buffer.buf< char >(), field, &fd );
+    int err = segy_set_field( buffer.buf< char >(), field, fd );
 
     switch( err ) {
         case SEGY_OK: