From c3a7328d3059b2d131115e74b33ac169d513f0bf Mon Sep 17 00:00:00 2001 From: Arthur LE MOIGNE Date: Wed, 17 Jan 2024 11:13:57 +0100 Subject: [PATCH] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20Make=20Message::as=5Fstrin?= =?UTF-8?q?g=20simpler?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../quickfix-bind/include/quickfix_bind.h | 3 +- .../quickfix-bind/src/quickfix_bind.cpp | 26 ++++++---- quickfix-ffi/src/lib.rs | 9 +++- quickfix/src/dictionary.rs | 11 ++-- quickfix/src/message.rs | 51 ++++++++++++++----- quickfix/src/utils.rs | 6 --- quickfix/tests/test_messages.rs | 12 +---- 7 files changed, 66 insertions(+), 52 deletions(-) diff --git a/quickfix-ffi/quickfix-bind/include/quickfix_bind.h b/quickfix-ffi/quickfix-bind/include/quickfix_bind.h index 0540b8f..001377e 100644 --- a/quickfix-ffi/quickfix-bind/include/quickfix_bind.h +++ b/quickfix-ffi/quickfix-bind/include/quickfix_bind.h @@ -134,7 +134,8 @@ const char *FixMessage_getField(const FixMessage_t *obj, int32_t tag); int8_t FixMessage_setField(FixMessage_t *obj, int32_t tag, const char *value); int8_t FixMessage_removeField(FixMessage_t *obj, int32_t tag); int8_t FixMessage_addGroup(FixMessage_t *obj, const FixGroup_t *group); -int8_t FixMessage_toBuffer(const FixMessage_t *obj, char *buffer, uint64_t length); +int64_t FixMessage_getStringLen(const FixMessage_t *obj); +int8_t FixMessage_readString(const FixMessage_t *obj, char *buffer, int64_t buffer_len); void FixMessage_delete(const FixMessage_t *obj); FixHeader_t *FixHeader_new(); diff --git a/quickfix-ffi/quickfix-bind/src/quickfix_bind.cpp b/quickfix-ffi/quickfix-bind/src/quickfix_bind.cpp index 29cbe47..1bd960d 100644 --- a/quickfix-ffi/quickfix-bind/src/quickfix_bind.cpp +++ b/quickfix-ffi/quickfix-bind/src/quickfix_bind.cpp @@ -281,21 +281,22 @@ int64_t FixDictionary_getStringLen(const Dictionary *obj, const char *key) { RETURN_VAL_IF_NULL(obj, ERRNO_INVAL); RETURN_VAL_IF_NULL(key, ERRNO_INVAL); - CATCH_OR_RETURN_ERRNO({ return obj->getString(key).size(); }) + CATCH_OR_RETURN_ERRNO({ return obj->getString(key).size() + 1; }) } int8_t FixDictionary_readString(const Dictionary *obj, const char *key, char *buffer, int64_t buffer_len) { RETURN_VAL_IF_NULL(obj, ERRNO_INVAL); RETURN_VAL_IF_NULL(key, ERRNO_INVAL); + RETURN_VAL_IF_NULL(buffer, ERRNO_INVAL); CATCH_OR_RETURN_ERRNO({ auto value = obj->getString(key); - if (buffer_len <= value.length()) { + if (buffer_len <= value.size()) { return ERRNO_BUFFER_TO_SMALL; } strncpy(buffer, value.c_str(), buffer_len); - buffer[value.length()] = '\0'; + buffer[value.size()] = '\0'; return 0; }) @@ -604,24 +605,27 @@ int8_t FixMessage_addGroup(Message *obj, const Group *group) { }) } -int8_t FixMessage_toBuffer(const Message *obj, char *buffer, uint64_t length) { - if (length == 0) - return 0; +int64_t FixMessage_getStringLen(const FixMessage_t *obj) { + RETURN_VAL_IF_NULL(obj, ERRNO_INVAL); + + CATCH_OR_RETURN_ERRNO({ return obj->toString().size() + 1; }); +} +int8_t FixMessage_readString(const FixMessage_t *obj, char *buffer, int64_t buffer_len) { RETURN_VAL_IF_NULL(obj, ERRNO_INVAL); RETURN_VAL_IF_NULL(buffer, ERRNO_INVAL); CATCH_OR_RETURN_ERRNO({ - auto repr = obj->toString(); - if (length <= repr.length()) { + auto value = obj->toString(); + if (buffer_len <= value.size()) { return ERRNO_BUFFER_TO_SMALL; } - strncpy(buffer, repr.c_str(), length); - buffer[repr.length()] = '\0'; + strncpy(buffer, value.c_str(), buffer_len); + buffer[value.size()] = '\0'; return 0; - }); + }) } void FixMessage_delete(const Message *obj) { diff --git a/quickfix-ffi/src/lib.rs b/quickfix-ffi/src/lib.rs index b7b5a8b..e235bc0 100644 --- a/quickfix-ffi/src/lib.rs +++ b/quickfix-ffi/src/lib.rs @@ -319,8 +319,15 @@ extern "C" { #[must_use] pub fn FixMessage_addGroup(obj: FixMessage_t, group: FixGroup_t) -> i8; + pub fn FixMessage_getStringLen(obj: FixMessage_t) -> i64; + #[must_use] - pub fn FixMessage_toBuffer(obj: FixMessage_t, buffer: *mut ffi::c_char, length: u64) -> i8; + pub fn FixMessage_readString( + obj: FixMessage_t, + buffer: *mut ffi::c_char, + buffer_len: i64, + ) -> i8; + pub fn FixMessage_delete(obj: FixMessage_t); // Header diff --git a/quickfix/src/dictionary.rs b/quickfix/src/dictionary.rs index 40ae487..e181c18 100644 --- a/quickfix/src/dictionary.rs +++ b/quickfix/src/dictionary.rs @@ -1,7 +1,4 @@ -use std::{ - ffi::{CStr, CString}, - fmt, -}; +use std::{ffi::CString, fmt}; use quickfix_ffi::{ FixDictionary_delete, FixDictionary_getBool, FixDictionary_getDay, FixDictionary_getDouble, @@ -61,13 +58,11 @@ impl PropertyContainer for Dictionary { fn ffi_get(&self, key: CString) -> Result { unsafe { // Prepare output buffer - let mut buffer_len = FixDictionary_getStringLen(self.0, key.as_ptr()); + let buffer_len = FixDictionary_getStringLen(self.0, key.as_ptr()); if buffer_len < 0 { return Err(QuickFixError::InvalidFunctionReturnCode(buffer_len as i8)); } - buffer_len += 1; // Add null end char - // Allocate buffer on rust side let mut buffer = vec![0_u8; buffer_len as usize]; assert_eq!(buffer.len(), buffer_len as usize); @@ -85,7 +80,7 @@ impl PropertyContainer for Dictionary { // NOTE: Here, I deliberately made the choice to drop C weird string / invalid UTF8 string // content. If this happen, there is not so much we can do about ... // Returning no error is sometime nicer, than an incomprehensible error. - let text = CStr::from_bytes_with_nul(&buffer).unwrap_or_default(); + let text = CString::from_vec_with_nul(buffer).unwrap_or_default(); Ok(text.to_string_lossy().to_string()) } } diff --git a/quickfix/src/message.rs b/quickfix/src/message.rs index 681f5ec..555f417 100644 --- a/quickfix/src/message.rs +++ b/quickfix/src/message.rs @@ -3,15 +3,15 @@ use std::{ffi::CString, fmt, mem::ManuallyDrop}; use quickfix_ffi::{ FixMessage_addGroup, FixMessage_copyGroup, FixMessage_copyHeader, FixMessage_copyTrailer, FixMessage_delete, FixMessage_fromString, FixMessage_getField, FixMessage_getGroupRef, - FixMessage_getHeaderRef, FixMessage_getTrailerRef, FixMessage_new, FixMessage_removeField, - FixMessage_setField, FixMessage_t, FixMessage_toBuffer, + FixMessage_getHeaderRef, FixMessage_getStringLen, FixMessage_getTrailerRef, FixMessage_new, + FixMessage_readString, FixMessage_removeField, FixMessage_setField, FixMessage_t, }; use crate::{ group::Group, header::Header, trailer::Trailer, - utils::{ffi_code_to_result, read_buffer_to_string, read_checked_cstr}, + utils::{ffi_code_to_result, read_checked_cstr}, AsFixValue, FieldMap, QuickFixError, }; @@ -32,18 +32,41 @@ impl Message { .ok_or(QuickFixError::NullFunctionReturn) } - /// Try reading underlying struct buffer as a string of 1 page size. + /// Try reading underlying struct buffer as a string. + /// + /// # Performances + /// + /// Do not use this method in latency sensitive code. + /// + /// String will be generated twice in C++ code: + /// - Once for getting a safe buffer length. + /// - Then to copy buffer to rust "memory". pub fn as_string(&self) -> Result { - self.as_string_with_len(4096 /* 1 page */) - } - - /// Try reading underlying struct buffer with a custom buffer size. - pub fn as_string_with_len(&self, max_len: usize) -> Result { - let mut buffer = vec![0_u8; max_len]; - let buffer_ptr = buffer.as_mut_ptr() as *mut i8; - match unsafe { FixMessage_toBuffer(self.0, buffer_ptr, max_len as u64) } { - 0 => Ok(read_buffer_to_string(&buffer)), - code => Err(QuickFixError::InvalidFunctionReturnCode(code)), + unsafe { + // Prepare output buffer + let buffer_len = FixMessage_getStringLen(self.0); + if buffer_len < 0 { + return Err(QuickFixError::InvalidFunctionReturnCode(buffer_len as i8)); + } + + // Allocate buffer on rust side + let mut buffer = vec![0_u8; buffer_len as usize]; + assert_eq!(buffer.len(), buffer_len as usize); + + // Read text + ffi_code_to_result(FixMessage_readString( + self.0, + buffer.as_mut_ptr().cast(), + buffer_len, + ))?; + + // Convert to String + // + // NOTE: Here, I deliberately made the choice to drop C weird string / invalid UTF8 string + // content. If this happen, there is not so much we can do about ... + // Returning no error is sometime nicer, than an incomprehensible error. + let text = CString::from_vec_with_nul(buffer).unwrap_or_default(); + Ok(text.to_string_lossy().to_string()) } } diff --git a/quickfix/src/utils.rs b/quickfix/src/utils.rs index eb50b53..45f73ef 100644 --- a/quickfix/src/utils.rs +++ b/quickfix/src/utils.rs @@ -5,12 +5,6 @@ use std::{ use crate::QuickFixError; -#[inline(always)] -pub fn read_buffer_to_string(buffer: &[u8]) -> String { - let null_index = buffer.iter().position(|x| *x == 0).unwrap_or(buffer.len()); - String::from_utf8_lossy(&buffer[..null_index]).to_string() -} - #[inline(always)] pub fn read_checked_cstr(val: NonNull) -> String { let cstr = unsafe { CStr::from_ptr(val.as_ptr()) }; diff --git a/quickfix/tests/test_messages.rs b/quickfix/tests/test_messages.rs index 5458c67..07d7c39 100644 --- a/quickfix/tests/test_messages.rs +++ b/quickfix/tests/test_messages.rs @@ -4,19 +4,9 @@ use quickfix::*; fn test_read_empy_message() { let msg = Message::new(); assert_eq!(msg.as_string().as_deref(), Ok("9=0\u{1}10=167\u{1}")); -} -#[test] -fn test_as_string() { let msg = Message::try_from_text("9=0\u{1}10=000\u{1}").unwrap(); - assert_eq!( - msg.as_string_with_len(512).as_deref(), - Ok("9=0\u{1}10=167\u{1}") - ); - assert_eq!( - msg.as_string_with_len(6), - Err(QuickFixError::InvalidFunctionReturnCode(-3)) - ); + assert_eq!(msg.as_string().as_deref(), Ok("9=0\u{1}10=167\u{1}")); } #[test]