Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

communications.c: Added support for messages spanning multiple lines. #66

Closed
wants to merge 1 commit into from

Conversation

kit-la
Copy link

@kit-la kit-la commented Aug 3, 2024

Changed receive_command() to read from the socket until the second occurrence of '\0' instead of waiting for the first \n, which previously made multi-line messages impossible. Now, messages containing '\0' will be truncated.

I would recommend to change the way this function reads from the socket; instead of reading one byte at a time, make the second and third byte sent to the socket by the server to be the size of the message. Then, the client determines the size it will need to parse and then either loop until the whole message has been read or read the entire message at once. I don't know enough C to implement this, but it would prevent from making so many read() calls as well as checking if len == sizeof(cmd) as many times as the size of the message.

Changed receive_command() to read from the socket until the second occurrence  of '\0' instead of waiting for the first \n, which previously made multi-line messages impossible. Now, messages containing '\0' will be truncated.

I would recommend to change the way this function reads from the socket; instead of reading one byte at a time, make the second and third byte sent to the socket by the server to be the size of the message. Then, the client determines the size it will need to parse and then either loop until the whole message has been read or read the entire message at once. I don't know enough C to implement this, but it would prevent from making so many read() calls as well as checking if len == sizeof(cmd) as many times as the size of the message.
@smemsh
Copy link
Contributor

smemsh commented Aug 4, 2024

Just curious, what do you have as input commands that require multiple lines?

It looks like send_command() currently creates the command string using \n as the terminator. Of course, in C, a string cannot contain a null byte. These terminate the string by definition.

The idea of including a length in the messaging protocol is a good one, but only if arbitrary data needs to come across. The overhead of multiple read() and length check is less than minuscule here. But I'm curious what application there is for multi-line command messages?

@kit-la
Copy link
Author

kit-la commented Aug 4, 2024

Just curious, what do you have as input commands that require multiple lines?

I have a ratpoison script that tells me some info, like battery percentage, uptime etc through ratpoison's echo command. It uses multiple lines, and I sort of prefer that way of showing info on demand rather than a bar, so if i couldn't implement it i wouldn't make the switch to sdorfehs.
Also, i have seen some reaaaally old animations made through ratpoison's echo and some scripting, really cool thingies, I think z3bra made them?? or i may have seen them at the ratpoison scripts github. cal(endar) also comes to mind.

It looks like send_command() currently creates the command string using \n as the terminator. Of course, in C, a string cannot contain a null byte. These terminate the string by definition.

So it would be possible to remove that bit from send_command() and also remove the bit to delete the \n? i didn't understand most of send_command(), so i went directly to receive_command(). Might try implementing it tomorrow, thank you!

@smemsh
Copy link
Contributor

smemsh commented Aug 4, 2024

I didn't know ratpoison echo could ever show multiple lines...

Based on the cmd_echo() implementation, it does appear that it was possible. The call path is cmd_echo -> marked_message_printf -> marked_message -> marked_message_internal which is counting lines in the message and then drawing a window ("bar") of appropriate size. Interesting, I didn't know about it. I have a bunch of things that use echo but I prefer my info on a single line. But I can see how it could be useful.

Did you actually build with your change and it works to allow multi-line echo ? how do you feed that to sdorfehs, like this?

sdorfehs -c $'echo foo\nbar\nbaz'

I don't think you can just remove the newline inserted by the snprintf() on the sender, because it's used as a sentinel value to tell the receiver it has reached the end of the command message. If you go until null, you could walk into junk at the end of the buffer. I suppose a null byte could be used as the sentinel though; if you initialized the whole BUFSZ buffer with memset() on the sending side you could just read until you saw one, as long as you were sure to send it over.

I'm guessing the receiver code reads one byte at a time to avoid having to deal with partial reads or care about lengths. However in theory you could do larger read()s and check the last byte to see if the message was complete. In practice, since the sender buffer is hardcoded not to exceed 1024 bytes, I doubt that would ever result in a short read, but I'm not sure about guarantees for unix domain sockets (as opposed to pipes).

To be robust, it would be even better to just keep looping and allow multiple buffers to be sent. I did this a while ago for the sender consuming the receiver's reply (as there were truncated sdorfehs -c windows outputs for example, with large numbers of windows), but didn't consider the initial sender's command buffer, as I didn't realize anything would be sending arbitrary data from that side.

Would everything you might want to input fit in a single 1024 byte buffer in practice?

@kit-la
Copy link
Author

kit-la commented Aug 4, 2024

Did you actually build with your change and it works to allow multi-line echo ? how do you feed that to sdorfehs, like this?

sdorfehs -c $'echo foo\nbar\nbaz'

Yes, i built it and it works. I tend to do something like

sdorfehs -c "echo $(printf 'jaja\njaja')"

from the command line, or to define a multi-line variable in scripts and then do

sdorfehs -c "echo ${var}"

But your way works too, and I like it much more than with printf, so i'll be using it from now on.

The new sentinel value in my commit, at least at receive_command() is \0. But i see that it is used as a sentinel value in a loop at send_message() too; i tried changing that just now, and although it works as intended, it spits error 104 at the same time as if count == -1. Don't really know why would spit an error and work as intended at the same time, it might have created some hidden bugs so it is not worth it i believe.

I don't think i will be sending anything bigger than 1024 bytes, but i can see at least an important use case that might need it, dumping and restoring frame data. The output generated through fdump and specially sfdump is really long. Assuming each new frame takes as much bytes in a dump as any other, fdump surpasses 1024 bytes at 8-9 frames, and although i wouldn't use 10 frames ever in a single screen, it might make sense to have 2+ screens with 4 frames each. In that case, if you try to use sfdump and then sfrestore, you would need more than a single 1024B buffer i think? i'm not really sure if that's how buffers work, but it sounds like it. I tried echoing a 1025 bytes file and it was truncated, so i think it is a problem. Another, weirder use case would be to just try sending a 1024 byte echo message, but that's pretty unlikely unless you want a really baroque message (or ascii art!).

@smemsh
Copy link
Contributor

smemsh commented Aug 5, 2024

You convinced me it should be done more robustly, allowing send of arbitrary message lengths. I will try to implement it.

@smemsh
Copy link
Contributor

smemsh commented Aug 19, 2024

I will try to implement it.

@kit-la please try #67 when you get a chance, thanks.

@kit-la
Copy link
Author

kit-la commented Aug 27, 2024

Superseded by #67. Thank you @smemsh!

@kit-la kit-la closed this Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants