[GitHub] [celix] PengZheng commented on a diff in pull request #459: Replace sprintf usage with snprintf or asprintf

[GitBox <gi...@apache.org>]

PengZheng commented on code in PR #459:
URL: https://github.com/apache/celix/pull/459#discussion_r1063265244


##########
bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c:
##########
@@ -1034,16 +1034,13 @@ int pubsub_tcpHandler_write(pubsub_tcpHandler_t *handle, pubsub_protocol_message
             message->header.payloadOffset = 0;
             message->header.isLastSegment = 1;
 
-            void *metadataData = NULL;
             size_t metadataSize = 0;
             if (message->metadata.metadata) {
-                metadataSize = entry->writeMetaBufferSize;
-                metadataData = entry->writeMetaBuffer;
+                handle->protocol->encodeMetadata(handle->protocol->handle, message, &entry->writeMetaBuffer, &entry->writeMetaBufferSize, &metadataSize);
                 // When maxMsgSize is smaller then meta data is disabled
-                if (metadataSize > entry->maxMsgSize) {
+                if (metadataSize + payloadSize > entry->maxMsgSize) {

Review Comment:
   I found the segmentation logic in this function very problematic:
   
   - Using the original condition, the size of the last segment carrying the last payload part AND the metadata can exceed `maxMsgSize`.
   - Using the current condition, we discard metadata for message with large payload, which should not happen for a proper segmentation implementation.
   - When comparing `payloadSize` and `maxMsgSize`, it fails to take header and footer into account.
   - `sendmsg` does not deal with partial transfer, which will make this implementation very unreliable.
   
   Though the current segmentation implementation is buggy, the buffer optimization is valid. 
   I suggest keeping the original wrong condition. Fixing this belong to a separate PR.
   
    ```suggestion
                   if (metadataSize > entry->maxMsgSize) {
   ```



##########
bundles/pubsub/pubsub_spi/include/pubsub_protocol.h:
##########
@@ -145,19 +151,24 @@ typedef struct pubsub_protocol_service {
     celix_status_t (*isMessageSegmentationSupported)(void *handle, bool *isSupported);
 
     /**
-     * Encodes the header using the supplied message.header.
+     * @brief Encode the header using the supplied message.header.
+     *
+     * If *bufferInOut is NULL a new buffer will be allocated.
+     * If *bufferInOut is not NULL and *bufferLengthInOut matches the header size the buffer will be reused.
+     * If *bufferInOut is not NULL, but *bufferLengthInOut does not match the header size, the provided buffer will
+     * be freed and a new buffer (matching the header size) will be allocated.
      *
      * @param handle handle for service
      * @param message message to use header from
-     * @param outBuffer byte array containing the encoded header
-     * @param outLength length of the byte array
+     * @param bufferInOut byte array containing the encoded header
+     * @param bufferLengthInOut length of the byte array
      * @return status code indicating failure or success
      */
-    celix_status_t (*encodeHeader)(void *handle, pubsub_protocol_message_t *message, void **outBuffer, size_t *outLength);
+    celix_status_t (*encodeHeader)(void *handle, pubsub_protocol_message_t *message, void **bufferInOut, size_t *bufferLengthInOut);
 
     /**
-     * Encodes the payload using the supplied message.header. Note, this decode is for protocol specific tasks, and does not perform
-     * the needed message serialization. See the serialization service for that.
+     * @brief Encode the payload using the supplied message.header. Note, this decode is for protocol specific tasks,

Review Comment:
   ```suggestion
        * @brief Encode the payload using the supplied message.header. Note, this encoding is for protocol specific tasks,
   ```
   
   Nitpick



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org