[Draft] FileType implementation #4436

opcua-tsn-team-kalycito · 2021-05-26T16:25:28Z

This pull request implements file implementation.

Methods implemented:
- Open, Close, Read, Write
- TODO: SetPosition, GetPosition
Properties supported
- Size, OpenCount

jpfr

Good feature!
See the inline comments.

include/open62541/server.h

jpfr

This is on a good way.
See above for a new batch of comments.

jpfr · 2021-06-10T05:22:29Z

include/open62541/server.h

+ * @return UA_STATUSCODE_GOOD on success
+ */
+UA_StatusCode UA_EXPORT
+UA_Server_addFile(UA_Server *server, const UA_NodeId requestedNewNodeId,


For consistency with registerFileNode, rename this to UA_Server_addFileNode.

jpfr · 2021-06-10T05:24:30Z

src/server/ua_file.c

@@ -0,0 +1,419 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public


Rename to ua_file_nodes.c.

jpfr · 2021-06-10T05:25:18Z

src/server/ua_file.c

+
+static UA_StatusCode
+updateOpenCountProperty(UA_Server *server, UA_FileType* fileObject)
+{


Use the usual conventions of the entire library with curly brackets on the same line.

jpfr · 2021-06-10T05:27:07Z

src/server/ua_file.c

+ UA_NODEID_NUMERIC(0, UA_NS0ID_FILETYPE),
+ attr, nodeContext, outNewNodeId);
+
+ UA_file *sampleFile = UA_file_open((char*)filePath.data, "w");


Error-check everywhere.
This should fail when the file does not exist.
Use proper logging,

This can only log an error and return the invalid UA_file* to the caller.

jpfr · 2021-06-10T05:28:53Z

src/server/ua_file.c

+ }
+
+ //else use the NodeId requested by user
+ else


This will fail if the user does not provider outNewNodeId and if the NodeId has identifier.numeric==0.
That is the convention to ask the nodestore to generate a new numeric nodeid.
Use a temporary newNodeId variable.

jpfr · 2021-06-10T05:30:51Z

src/server/ua_file.c

+{
+ UA_UInt32 fileHandle = *(UA_UInt32 *)input[0].data;
+ UA_Int32 value = *(UA_Int32 *)input[1].data;
+ UA_UInt64 requestedLengthToRead = (UA_UInt64)value;


Use size_t for lengthToRead etc.

jpfr · 2021-06-10T05:39:53Z

src/server/ua_file.c

+ size_t inputSize, const UA_Variant *input,
+ size_t outputSize, UA_Variant *output)
+{
+ return UA_STATUSCODE_BADNOTIMPLEMENTED;


The idea of a file is to transport content that is larger than what fits inside a single message.
You can only do that if you preserve the position in the file across different calls to read.

So you need some persistency of that information stored in the user session.
There is a way to get there in a clean fashion. The new PR #4463 adds a key-value configuration system.
Add key-value parameters to the session struct.
And select a nice key-name under which the position for each particular file is stored.

Also limit the number of open files per session.
Or this can be a way to consume arbitrary amounts of memory.

what would be the recommended default value for max open files per session ?

The idea of a file is to transport content that is larger than what fits inside a single message. You can only do that if you preserve the position in the file across different calls to read.

So you need some persistency of that information stored in the user session. There is a way to get there in a clean fashion. The new PR #4463 adds a key-value configuration system. Add key-value parameters to the session struct. And select a nice key-name under which the position for each particular file is stored.

Is the persistency of the current position of file descriptor in the session really needed here ? Because this seems to be implied by the current implementation anyway. As long the Close is not called, each subsequent call to Read from the same FileHandle will read from the it's underlying FILE* stream starting from it's current stream position, which is advanced by every read operation.

jpfr · 2021-06-10T05:43:19Z

src/server/ua_server.c

@@ -171,6 +171,24 @@ void UA_Server_delete(UA_Server *server) {
 }
 UA_Array_delete(server->namespaces, server->namespacesSize, &UA_TYPES[UA_TYPES_STRING]);

+#ifdef UA_ENABLE_FILETYPE_OBJECT_SUPPORT
+ UA_FileType *fileObject, *fileObject_tmp;


Please keep this as much out of ua_server.c as possible.
Set up destructor set for the FileType object type.
So the cleanup happens automatically when the node is deleted.

jpfr · 2021-06-10T05:44:48Z

src/server/ua_server_internal.h

+
+typedef struct UA_FileType {
+ LIST_ENTRY(UA_FileType) listEntry;
+ UA_NodeId fileTypeObjectNodeId;


fileTypeObjectNodeId or fileNodeId?

Why a list of filetyeobjects in the ua_server_internal.h is necessary?

jpfr · 2021-06-11T07:19:39Z

You can now use this facility to attach data to session:
https://github.com/open62541/open62541/blob/master/include/open62541/server.h#L434

ZidCode · 2021-09-15T14:59:28Z

Hi,

just wanted to ask what is the current status of this ticket?

Cheers

ZidCode · 2021-09-24T10:33:59Z

src/server/ua_file.c

+ result |= getFieldNodeId(server, &fileObjectNodeId, &fieldSetPositionQN, &setPositionNodeId);
+ result |= getFieldNodeId(server, &fileObjectNodeId, &fieldGetPositionQN, &getPositionNodeId);
+ result |= getFieldNodeId(server, &fileObjectNodeId, &fieldWriteQN, &writeNodeId);
+ result |= getFieldNodeId(server, &fileObjectNodeId, &fieldCloseQN, &closeNodeId);


This always returns the NodeIds of the Type Object. Shouldn't it be the NodeID of the Object itself? Is it necessary to add explicitly the methods to the Object?

- Includes support for open, read, write, close file methods operations - TODO: GetPosition and setPosition method implementation Signed-off-by: Jayanth Velusamy <[email protected]>

Signed-off-by: Jayanth Velusamy <[email protected]>

daniel-petrovic · 2022-07-15T08:25:37Z

src/server/ua_file.c

+ }
+ readBuffer->length = (size_t)lengthToRead;
+ readBuffer->data = (UA_Byte*) UA_malloc(readBuffer->length * sizeof(UA_Byte));
+ UA_file_read(readBuffer->data, (readBuffer->length + 1), 1, fileInfo->file);


error check of UA_file_read missing

daniel-petrovic · 2022-07-15T10:12:13Z

Hi,

just wanted to ask what is the current status of this ticket?

Cheers

Would also like to know that ... have addressed some of the open issues in this PR but I'm not sure if it's still maintained

pberginkonsult · 2023-07-03T14:12:25Z

Hi,
just wanted to ask what is the current status of this ticket?
Cheers

Would also like to know that ... have addressed some of the open issues in this PR but I'm not sure if it's still maintained

And another one that wants to know the status of this PR. Is it something that can be accepted or is it rejected for inclusion in the project?

daniel-petrovic · 2024-01-26T10:07:10Z

src/server/ua_server.c

+ UA_FileType *fileObject, *fileObject_tmp;
+ LIST_FOREACH_SAFE(fileObject, &server->fileObjects, listEntry, fileObject_tmp)
+ {
+ UA_Server_deleteNode(server, fileObject->fileNodeId, true);


This will likely cause the UA_assert(++(lock->mutexCounter) == 1); in UA_LOCK which is called in UA_Server_deleteNode to fail. ( Note: The UA_LOCK_INIT sets the mutex attribute PTHREAD_MUTEX_RECURSIVE).

giokara-oqton · 2024-07-05T14:37:13Z

One more interested party here. Kudos to @daniel-petrovic for making the PR. Would really appreciate seeing this merged.

opcua-tsn-team-kalycito mentioned this pull request May 26, 2021

Implementation of FileType and friends #2383

Open

opcua-tsn-team-kalycito force-pushed the fileType_object_implementation branch 3 times, most recently from 8adb120 to 5945e09 Compare May 27, 2021 08:52

opcua-tsn-team-kalycito changed the title ~~[WIP] File implementation~~ [REVIEW] FileType implementation May 27, 2021

jpfr requested changes May 27, 2021

View reviewed changes

include/open62541/server.h Outdated Show resolved Hide resolved

include/open62541/server.h Show resolved Hide resolved

opcua-tsn-team-kalycito changed the title ~~[REVIEW] FileType implementation~~ [WIP] FileType implementation May 28, 2021

opcua-tsn-team-kalycito force-pushed the fileType_object_implementation branch 2 times, most recently from df6e19d to a133a29 Compare June 8, 2021 13:52

opcua-tsn-team-kalycito changed the title ~~[WIP] FileType implementation~~ [REVIEW] FileType implementation Jun 8, 2021

opcua-tsn-team-kalycito requested a review from jpfr June 9, 2021 15:55

jpfr requested changes Jun 10, 2021

View reviewed changes

opcua-tsn-team-kalycito changed the title ~~[REVIEW] FileType implementation~~ [WIP] FileType implementation Jun 10, 2021

ZidCode reviewed Sep 24, 2021

View reviewed changes

opcua-tsn-team-kalycito force-pushed the fileType_object_implementation branch from a133a29 to e4c11a9 Compare December 3, 2021 11:38

opcua-tsn-team-kalycito added 2 commits December 3, 2021 17:16

feat(server) Support for fileType Object

76eb14f

- Includes support for open, read, write, close file methods operations - TODO: GetPosition and setPosition method implementation Signed-off-by: Jayanth Velusamy <[email protected]>

test(server) Unit test for fileType object instance

8d34204

Signed-off-by: Jayanth Velusamy <[email protected]>

opcua-tsn-team-kalycito force-pushed the fileType_object_implementation branch from e4c11a9 to 8d34204 Compare December 3, 2021 11:49

andreasebner changed the title ~~[WIP] FileType implementation~~ [Draft] FileType implementation May 10, 2022

andreasebner marked this pull request as draft May 10, 2022 14:28

daniel-petrovic mentioned this pull request Jul 15, 2022

Feature/filetype opcua-tsn-team-kalycito/open62541#21

Closed

daniel-petrovic reviewed Jul 15, 2022

View reviewed changes

daniel-petrovic mentioned this pull request Jul 15, 2022

Fixed some issues in #PR 4436 opcua-tsn-team-kalycito/open62541#22

Open

daniel-petrovic reviewed Jan 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Draft] FileType implementation #4436

[Draft] FileType implementation #4436

opcua-tsn-team-kalycito commented May 26, 2021

jpfr left a comment

jpfr left a comment

jpfr Jun 10, 2021

jpfr Jun 10, 2021

jpfr Jun 10, 2021

jpfr Jun 10, 2021

suy Jul 18, 2022

jpfr Jun 10, 2021

jpfr Jun 10, 2021

jpfr Jun 10, 2021

jpfr Jun 10, 2021

daniel-petrovic Jul 12, 2022 •

edited

Loading

daniel-petrovic Jul 15, 2022 •

edited

Loading

jpfr Jun 10, 2021

jpfr Jun 10, 2021

ZidCode Sep 17, 2021

jpfr commented Jun 11, 2021

ZidCode commented Sep 15, 2021

ZidCode Sep 24, 2021

daniel-petrovic Jul 15, 2022

daniel-petrovic commented Jul 15, 2022

pberginkonsult commented Jul 3, 2023

daniel-petrovic Jan 26, 2024 •

edited

Loading

giokara-oqton commented Jul 5, 2024

		@@ -0,0 +1,419 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

[Draft] FileType implementation #4436

Are you sure you want to change the base?

[Draft] FileType implementation #4436

Conversation

opcua-tsn-team-kalycito commented May 26, 2021

jpfr left a comment

Choose a reason for hiding this comment

jpfr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-petrovic Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

daniel-petrovic Jul 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpfr commented Jun 11, 2021

ZidCode commented Sep 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-petrovic commented Jul 15, 2022

pberginkonsult commented Jul 3, 2023

daniel-petrovic Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

giokara-oqton commented Jul 5, 2024

daniel-petrovic Jul 12, 2022 •

edited

Loading

daniel-petrovic Jul 15, 2022 •

edited

Loading

daniel-petrovic Jan 26, 2024 •

edited

Loading