refactor to remove gotos

src/server/ua_services_attribute.c

@@ -391,8 +391,11 @@ writeValueRankAttribute(UA_Server *server, UA_VariableNode *node, UA_Int32 value
         retval = readValueAttribute(server, node, &value);
         if(retval != UA_STATUSCODE_GOOD)
             return retval;
-        if(!value.hasValue || value.value.data == NULL)
-            goto apply; /* no value or null array */
+        if(!value.hasValue || !value.value.type) {
+            /* no value -> apply */
+            node->valueRank = valueRank;
+            return UA_STATUSCODE_GOOD;
+        }
         if(!UA_Variant_isScalar(&value.value))
             arrayDims = 1;
         UA_DataValue_deleteMembers(&value);
@@ -402,7 +405,6 @@ writeValueRankAttribute(UA_Server *server, UA_VariableNode *node, UA_Int32 value
         return retval;
 
     /* All good, apply the change */
- apply:
     node->valueRank = valueRank;
     return UA_STATUSCODE_GOOD;
 }
@@ -602,8 +604,11 @@ writeValueAttribute(UA_Server *server, UA_VariableNode *node,
         retval = typeCheckValue(server, &node->dataType, node->valueRank,
                                 node->arrayDimensionsSize, node->arrayDimensions,
                                 &value->value, rangeptr, &editableValue.value);
-        if(retval != UA_STATUSCODE_GOOD)
-            goto cleanup;
+        if(retval != UA_STATUSCODE_GOOD) {
+            if(rangeptr)
+                UA_free(range.dimensions);
+            return retval;
+        }
     }
 
     /* Set the source timestamp if there is none */
@@ -647,7 +652,6 @@ writeValueAttribute(UA_Server *server, UA_VariableNode *node,
     }
 
     /* Clean up */
- cleanup:
     if(rangeptr)
         UA_free(range.dimensions);
     return retval;

src/server/ua_services_nodemanagement.c

@@ -82,55 +82,32 @@ checkParentReference(UA_Server *server, UA_Session *session, UA_NodeClass nodeCl
     return UA_STATUSCODE_GOOD;
 }
 
-/* Check the consistency of the variable (or variable type) attributes data
- * type, value rank, array dimensions internally and against the parent variable
- * type. */
 static UA_StatusCode
-typeCheckVariableNode(UA_Server *server, UA_Session *session,
-                      const UA_VariableNode *node,
-                      const UA_NodeId *typeDef) {
-    /* Omit some type checks for ns0 generation */
-    const UA_NodeId baseDataVariableType =
-        UA_NODEID_NUMERIC(0, UA_NS0ID_BASEDATAVARIABLETYPE);
-    if(UA_NodeId_equal(&node->nodeId, &baseDataVariableType))
-        return UA_STATUSCODE_GOOD;
-
-    /* Get the variable type */
-    const UA_VariableTypeNode *vt =
-        (const UA_VariableTypeNode*)UA_NodeStore_get(server->nodestore, typeDef);
-    if(!vt || vt->nodeClass != UA_NODECLASS_VARIABLETYPE)
-        return UA_STATUSCODE_BADTYPEDEFINITIONINVALID;
-    if(node->nodeClass == UA_NODECLASS_VARIABLE && vt->isAbstract)
-        return UA_STATUSCODE_BADTYPEDEFINITIONINVALID;
-
-    /* We need the value for some checks. Might come from a datasource. */
-    UA_DataValue value;
-    UA_DataValue_init(&value);
-    UA_StatusCode retval = readValueAttribute(server, node, &value);
-    if(retval != UA_STATUSCODE_GOOD)
-        return retval;
-
+typeCheckVariableNodeWithValue(UA_Server *server, UA_Session *session,
+                               const UA_VariableNode *node,
+                               const UA_VariableTypeNode *vt,
+                               UA_DataValue *value) {
     /* Workaround: set a sane valueRank (the most permissive -2) */
     UA_Int32 valueRank = node->valueRank;
-    if(valueRank == 0 && value.hasValue && value.value.type &&
-       UA_Variant_isScalar(&value.value)) {
+    if(valueRank == 0 && value->hasValue && value->value.type &&
+       UA_Variant_isScalar(&value->value)) {
         UA_LOG_INFO_SESSION(server->config.logger, session,
                             "AddNodes: Use a default ValueRank of -2");
         valueRank = -2;
-        retval = UA_Server_writeValueRank(server, node->nodeId, valueRank);
+        UA_StatusCode retval = UA_Server_writeValueRank(server, node->nodeId, valueRank);
         if(retval != UA_STATUSCODE_GOOD)
-            goto cleanup;
+            return retval;
     }
 
     /* If no value is set, see if the vt provides one and copy that. THis needs to be done
      * before copying the datatype from the vt. Setting the datatype triggers the
      * typecheck. Here, we have only a typecheck when the datatype is already not null. */
-    if(!value.hasValue || !value.value.type) {
-        retval = readValueAttribute(server, (const UA_VariableNode*)vt, &value);
-        if(retval == UA_STATUSCODE_GOOD && value.hasValue && value.value.type)
-            retval = UA_Server_writeValue(server, node->nodeId, value.value);
+    if(!value->hasValue || !value->value.type) {
+        UA_StatusCode retval = readValueAttribute(server, (const UA_VariableNode*)vt, value);
+        if(retval == UA_STATUSCODE_GOOD && value->hasValue && value->value.type)
+            retval = UA_Server_writeValue(server, node->nodeId, value->value);
         if(retval != UA_STATUSCODE_GOOD)
-            goto cleanup;
+            return retval;
     }
 
     /* Workaround: Replace with datatype of the vt if not set */
@@ -139,53 +116,83 @@ typeCheckVariableNode(UA_Server *server, UA_Session *session,
         UA_LOG_INFO_SESSION(server->config.logger, session, "AddNodes: "
                             "Use a default DataType (from the TypeDefinition)");
         dataType = &vt->dataType;
-        retval = UA_Server_writeDataType(server, node->nodeId, vt->dataType);
+        UA_StatusCode retval = UA_Server_writeDataType(server, node->nodeId, vt->dataType);
         if(retval != UA_STATUSCODE_GOOD)
-            goto cleanup;
+            return retval;
     }
 
     /* Check the datatype against the vt */
-    if(!compatibleDataType(server, dataType, &vt->dataType)) {
-        retval = UA_STATUSCODE_BADTYPEMISMATCH;
-        goto cleanup;
-    }
+    if(!compatibleDataType(server, dataType, &vt->dataType))
+        return UA_STATUSCODE_BADTYPEMISMATCH;
 
     /* Get the array dimensions */
     size_t arrayDims = node->arrayDimensionsSize;
-    if(arrayDims == 0 && value.hasValue && value.value.type &&
-       !UA_Variant_isScalar(&value.value)) {
+    if(arrayDims == 0 && value->hasValue && value->value.type &&
+       !UA_Variant_isScalar(&value->value)) {
         arrayDims = 1; /* No array dimensions on an array implies one dimension */
     }
 
     /* Check valueRank against array dimensions */
-    retval = compatibleValueRankArrayDimensions(valueRank, arrayDims);
+    UA_StatusCode retval = compatibleValueRankArrayDimensions(valueRank, arrayDims);
     if(retval != UA_STATUSCODE_GOOD)
-        goto cleanup;
+        return retval;
 
     /* Check valueRank against the vt */
     retval = compatibleValueRanks(valueRank, vt->valueRank);
     if(retval != UA_STATUSCODE_GOOD)
-        goto cleanup;
+        return retval;
 
     /* Check array dimensions against the vt */
     retval = compatibleArrayDimensions(node->arrayDimensionsSize, node->arrayDimensions,
                                        vt->arrayDimensionsSize, vt->arrayDimensions);
     if(retval != UA_STATUSCODE_GOOD)
-        goto cleanup;
+        return retval;
 
     /* Typecheck the value */
-    if(value.hasValue) {
+    if(value->hasValue) {
         retval = typeCheckValue(server, dataType, valueRank,
                                 node->arrayDimensionsSize, node->arrayDimensions,
-                                &value.value, NULL, NULL);
+                                &value->value, NULL, NULL);
         /* The type-check failed. Write the same value again. The write-service
          * tries to convert to the correct type... */
         if(retval != UA_STATUSCODE_GOOD)
-            retval = UA_Server_writeValue(server, node->nodeId, value.value);
+            retval = UA_Server_writeValue(server, node->nodeId, value->value);
     }
+    return retval;
+}
+
+/* Check the consistency of the variable (or variable type) attributes data
+ * type, value rank, array dimensions internally and against the parent variable
+ * type. */
+static UA_StatusCode
+typeCheckVariableNode(UA_Server *server, UA_Session *session,
+                      const UA_VariableNode *node,
+                      const UA_NodeId *typeDef) {
+    /* Omit some type checks for ns0 generation */
+    const UA_NodeId baseDataVariableType =
+        UA_NODEID_NUMERIC(0, UA_NS0ID_BASEDATAVARIABLETYPE);
+    if(UA_NodeId_equal(&node->nodeId, &baseDataVariableType))
+        return UA_STATUSCODE_GOOD;
+
+    /* Get the variable type */
+    const UA_VariableTypeNode *vt =
+        (const UA_VariableTypeNode*)UA_NodeStore_get(server->nodestore, typeDef);
+    if(!vt || vt->nodeClass != UA_NODECLASS_VARIABLETYPE)
+        return UA_STATUSCODE_BADTYPEDEFINITIONINVALID;
+    if(node->nodeClass == UA_NODECLASS_VARIABLE && vt->isAbstract)
+        return UA_STATUSCODE_BADTYPEDEFINITIONINVALID;
+
+    /* We need the value for some checks. Might come from a datasource, so we perform a
+     * regular read. */
+    UA_DataValue value;
+    UA_DataValue_init(&value);
+    UA_StatusCode retval = readValueAttribute(server, node, &value);
+    if(retval != UA_STATUSCODE_GOOD)
+        return retval;
+
+    retval = typeCheckVariableNodeWithValue(server, session, node, vt, &value);
 
- cleanup:
-    UA_DataValue_deleteMembers(&value); /* Free the value before any return clause */
+    UA_DataValue_deleteMembers(&value);
     return retval;
 }
 

src/server/ua_subscription.c

@@ -67,11 +67,36 @@ ensureSpaceInMonitoredItemQueue(UA_MonitoredItem *mon) {
     --mon->currentQueueSize;
 }
 
+/* Errors are returned as no change detected */
+static UA_Boolean
+detectValueChangeWithFilter(UA_MonitoredItem *mon, UA_DataValue *value,
+                            UA_ByteString *encoding) {
+    /* Encode the data for comparison */
+    size_t binsize = UA_calcSizeBinary(value, &UA_TYPES[UA_TYPES_DATAVALUE]);
+    if(binsize == 0)
+        return false;
+
+    /* Allocate buffer on the heap if necessary */
+    if(binsize > UA_VALUENCODING_MAXSTACK &&
+       UA_ByteString_allocBuffer(encoding, binsize) != UA_STATUSCODE_GOOD)
+        return false;
+
+    /* Encode the value */
+    size_t encodingOffset = 0;
+    UA_StatusCode retval = UA_encodeBinary(value, &UA_TYPES[UA_TYPES_DATAVALUE],
+                                           NULL, NULL, encoding, &encodingOffset);
+    if(retval != UA_STATUSCODE_GOOD)
+        return false;
+
+    /* The value has changed */
+    encoding->length = encodingOffset;
+    return !mon->lastSampledValue.data || !UA_String_equal(encoding, &mon->lastSampledValue);
+}
+
 /* Has this sample changed from the last one? The method may allocate additional
  * space for the encoding buffer. Detect the change in encoding->data. */
-static UA_StatusCode
-detectValueChange(UA_MonitoredItem *mon, UA_DataValue *value,
-                  UA_ByteString *encoding, UA_Boolean *changed) {
+static UA_Boolean
+detectValueChange(UA_MonitoredItem *mon, UA_DataValue *value, UA_ByteString *encoding) {
     /* Apply Filter */
     UA_Boolean hasValue = value->hasValue;
     if(mon->trigger == UA_DATACHANGETRIGGER_STATUS)
@@ -87,125 +112,70 @@ detectValueChange(UA_MonitoredItem *mon, UA_DataValue *value,
         value->hasSourcePicoseconds = false;
     }
 
-    /* Forward declare before goto */
-    UA_StatusCode retval = UA_STATUSCODE_GOOD;
-    size_t encodingOffset = 0;
-    size_t binsize;
-
-    /* Encode the data for comparison */
-    binsize = UA_calcSizeBinary(value, &UA_TYPES[UA_TYPES_DATAVALUE]);
-    if(binsize == 0) {
-        retval = UA_STATUSCODE_BADINTERNALERROR;
-        goto cleanup;
-    }
-
-    /* Allocate buffer on the heap if necessary */
-    if(binsize > UA_VALUENCODING_MAXSTACK &&
-       UA_ByteString_allocBuffer(encoding, binsize) != UA_STATUSCODE_GOOD) {
-        retval = UA_STATUSCODE_BADOUTOFMEMORY;
-        goto cleanup;
-    }
-
-    /* Encode the value */
-    retval = UA_encodeBinary(value, &UA_TYPES[UA_TYPES_DATAVALUE],
-                             NULL, NULL, encoding, &encodingOffset);
-    if(retval != UA_STATUSCODE_GOOD)
-        goto cleanup;
-
-    /* The value has changed */
-    encoding->length = encodingOffset;
-    if(!mon->lastSampledValue.data || !UA_String_equal(encoding, &mon->lastSampledValue))
-        *changed = true;
+    /* Detect the Value Change */
+    UA_Boolean res = detectValueChangeWithFilter(mon, value, encoding);
 
- cleanup:
     /* Reset the filter */
     value->hasValue = hasValue;
     value->hasServerTimestamp = hasServerTimestamp;
     value->hasServerPicoseconds = hasServerPicoseconds;
     value->hasSourceTimestamp = hasSourceTimestamp;
     value->hasSourcePicoseconds = hasSourcePicoseconds;
-    return retval;
+    return res;
 }
 
-void UA_MoniteredItem_SampleCallback(UA_Server *server, UA_MonitoredItem *monitoredItem) {
-    UA_Subscription *sub = monitoredItem->subscription;
-    if(monitoredItem->monitoredItemType != UA_MONITOREDITEMTYPE_CHANGENOTIFY) {
-        UA_LOG_DEBUG_SESSION(server->config.logger, sub->session,
-                             "Subscription %u | MonitoredItem %i | "
-                             "Not a data change notification",
-                             sub->subscriptionID, monitoredItem->itemId);
-        return;
-    }
-
-    /* Adjust timestampstoreturn to get source timestamp for triggering */
-    UA_TimestampsToReturn ts = monitoredItem->timestampsToReturn;
-    if(ts == UA_TIMESTAMPSTORETURN_SERVER)
-        ts = UA_TIMESTAMPSTORETURN_BOTH;
-    else if(ts == UA_TIMESTAMPSTORETURN_NEITHER)
-        ts = UA_TIMESTAMPSTORETURN_SOURCE;
-
-    /* Read the value */
-    UA_ReadValueId rvid;
-    UA_ReadValueId_init(&rvid);
-    rvid.nodeId = monitoredItem->monitoredNodeId;
-    rvid.attributeId = monitoredItem->attributeID;
-    rvid.indexRange = monitoredItem->indexRange;
-    UA_DataValue value;
-    UA_DataValue_init(&value);
-    Service_Read_single(server, sub->session, ts, &rvid, &value);
-
-    /* Stack-allocate some memory for the value encoding */
-    UA_Byte *stackValueEncoding = (UA_Byte *)UA_alloca(UA_VALUENCODING_MAXSTACK);
-    UA_ByteString valueEncoding;
-    valueEncoding.data = stackValueEncoding;
-    valueEncoding.length = UA_VALUENCODING_MAXSTACK;
-
-    /* Forward declaration before goto */
-    MonitoredItem_queuedValue *newQueueItem;
+/* Returns whether a new sample was created */
+static UA_Boolean
+sampleCallbackWithValue(UA_Server *server, UA_Subscription *sub,
+                        UA_MonitoredItem *monitoredItem,
+                        UA_DataValue *value, UA_ByteString *valueEncoding) {
+    /* Store the pointer to the stack-allocated bytestring to see if a heap-allocation
+     * was necessary */
+    UA_Byte *stackValueEncoding = valueEncoding->data;
 
     /* Has the value changed? */
-    UA_Boolean changed = false;
-    UA_StatusCode retval = detectValueChange(monitoredItem, &value,
-                                             &valueEncoding, &changed);
-    if(!changed || retval != UA_STATUSCODE_GOOD)
-        goto cleanup;
+    UA_Boolean changed = detectValueChange(monitoredItem, value, valueEncoding);
+    if(!changed)
+        return false;
 
     /* Allocate the entry for the publish queue */
-    newQueueItem = (MonitoredItem_queuedValue *)UA_malloc(sizeof(MonitoredItem_queuedValue));
+    MonitoredItem_queuedValue *newQueueItem =
+        (MonitoredItem_queuedValue *)UA_malloc(sizeof(MonitoredItem_queuedValue));
     if(!newQueueItem) {
         UA_LOG_WARNING_SESSION(server->config.logger, sub->session,
                                "Subscription %u | MonitoredItem %i | "
                                "Item for the publishing queue could not be allocated",
                                sub->subscriptionID, monitoredItem->itemId);
-        goto cleanup;
+        return false;
     }
 
     /* Copy valueEncoding on the heap for the next comparison (if not already done) */
-    if(valueEncoding.data == stackValueEncoding) {
+    if(valueEncoding->data == stackValueEncoding) {
         UA_ByteString cbs;
-        if(UA_ByteString_copy(&valueEncoding, &cbs) != UA_STATUSCODE_GOOD) {
+        if(UA_ByteString_copy(valueEncoding, &cbs) != UA_STATUSCODE_GOOD) {
             UA_LOG_WARNING_SESSION(server->config.logger, sub->session,
                                    "Subscription %u | MonitoredItem %i | "
                                    "ByteString to compare values could not be created",
                                    sub->subscriptionID, monitoredItem->itemId);
             UA_free(newQueueItem);
-            goto cleanup;
+            return false;
         }
-        valueEncoding = cbs;
+        *valueEncoding = cbs;
     }
 
     /* Prepare the newQueueItem */
-    if(value.hasValue && value.value.storageType == UA_VARIANT_DATA_NODELETE) {
-        if(UA_DataValue_copy(&value, &newQueueItem->value) != UA_STATUSCODE_GOOD) {
+    if(value->hasValue && value->value.storageType == UA_VARIANT_DATA_NODELETE) {
+        /* Make a deep copy of the value */
+        if(UA_DataValue_copy(value, &newQueueItem->value) != UA_STATUSCODE_GOOD) {
             UA_LOG_WARNING_SESSION(server->config.logger, sub->session,
                                    "Subscription %u | MonitoredItem %i | "
                                    "Item for the publishing queue could not be prepared",
                                    sub->subscriptionID, monitoredItem->itemId);
             UA_free(newQueueItem);
-            goto cleanup;
+            return false;
         }
     } else {
-        newQueueItem->value = value;
+        newQueueItem->value = *value; /* Just copy the value and do not release it */
     }
     newQueueItem->clientHandle = monitoredItem->clientHandle;
 
@@ -217,18 +187,60 @@ void UA_MoniteredItem_SampleCallback(UA_Server *server, UA_MonitoredItem *monito
 
     /* Replace the encoding for comparison */
     UA_ByteString_deleteMembers(&monitoredItem->lastSampledValue);
-    monitoredItem->lastSampledValue = valueEncoding;
+    monitoredItem->lastSampledValue = *valueEncoding;
 
     /* Add the sample to the queue for publication */
     ensureSpaceInMonitoredItemQueue(monitoredItem);
     TAILQ_INSERT_TAIL(&monitoredItem->queue, newQueueItem, listEntry);
     ++monitoredItem->currentQueueSize;
-    return;
+    return true;;
+}
+
+void UA_MoniteredItem_SampleCallback(UA_Server *server, UA_MonitoredItem *monitoredItem) {
+    UA_Subscription *sub = monitoredItem->subscription;
+    if(monitoredItem->monitoredItemType != UA_MONITOREDITEMTYPE_CHANGENOTIFY) {
+        UA_LOG_DEBUG_SESSION(server->config.logger, sub->session,
+                             "Subscription %u | MonitoredItem %i | "
+                             "Not a data change notification",
+                             sub->subscriptionID, monitoredItem->itemId);
+        return;
+    }
 
- cleanup:
-    if(valueEncoding.data != stackValueEncoding)
-        UA_ByteString_deleteMembers(&valueEncoding);
-    UA_DataValue_deleteMembers(&value);
+    /* Adjust timestampstoreturn to get source timestamp for triggering */
+    UA_TimestampsToReturn ts = monitoredItem->timestampsToReturn;
+    if(ts == UA_TIMESTAMPSTORETURN_SERVER)
+        ts = UA_TIMESTAMPSTORETURN_BOTH;
+    else if(ts == UA_TIMESTAMPSTORETURN_NEITHER)
+        ts = UA_TIMESTAMPSTORETURN_SOURCE;
+
+    /* Read the value */
+    UA_ReadValueId rvid;
+    UA_ReadValueId_init(&rvid);
+    rvid.nodeId = monitoredItem->monitoredNodeId;
+    rvid.attributeId = monitoredItem->attributeID;
+    rvid.indexRange = monitoredItem->indexRange;
+    UA_DataValue value;
+    UA_DataValue_init(&value);
+    Service_Read_single(server, sub->session, ts, &rvid, &value);
+
+    /* Stack-allocate some memory for the value encoding. We might heap-allocate
+     * more memory if needed. This is just enough for scalars and small
+     * structures. */
+    UA_Byte *stackValueEncoding = (UA_Byte *)UA_alloca(UA_VALUENCODING_MAXSTACK);
+    UA_ByteString valueEncoding;
+    valueEncoding.data = stackValueEncoding;
+    valueEncoding.length = UA_VALUENCODING_MAXSTACK;
+
+    /* Create a sample and compare with the last value */
+    UA_Boolean newNotification = sampleCallbackWithValue(server, sub, monitoredItem,
+                                                         &value, &valueEncoding);
+
+    /* Clean up */
+    if(!newNotification) {
+        if(valueEncoding.data != stackValueEncoding)
+            UA_ByteString_deleteMembers(&valueEncoding);
+        UA_DataValue_deleteMembers(&value);
+    }
 }
 
 UA_StatusCode
@@ -380,7 +392,7 @@ prepareNotificationMessage(UA_Subscription *sub, UA_NotificationMessage *message
                            size_t notifications) {
     /* Array of ExtensionObject to hold different kinds of notifications
        (currently only DataChangeNotifications) */
-    message->notificationData = (UA_ExtensionObject *)UA_Array_new(1, &UA_TYPES[UA_TYPES_EXTENSIONOBJECT]);
+    message->notificationData = UA_ExtensionObject_new();
     if(!message->notificationData)
         return UA_STATUSCODE_BADOUTOFMEMORY;
     message->notificationDataSize = 1;