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

Integrate linenoise with web server #162

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

vax-r
Copy link
Contributor

@vax-r vax-r commented Feb 29, 2024

Summary

In the previous version of console implementation, we tried to integrate tiny-web-server to enable the ability of processing commands from web requests. As the result, the package linenoise which is responsible for
command-line auto-complete needs to be disabled during the running time of tiny-web-server. Because the line_edit() function in linenoise.c doesn't have the ability to handle web requests correctly.

When we start the web server, we use cmd_select in console.c and use select system call to monitor web socket file descriptor and stdin_fd at the same time, however, this ability should present in the line_edit function in linenoise.c so we can process commands from command-line and from web requests at the same time.

That's the reason I re-design the linenoise implementation and make some modification to put select() system call in line_edit so we can have the full ability to use web server and linenoise package in command-line at the same time.

@vax-r vax-r force-pushed the Integrate_linenoise_web branch 2 times, most recently from 4fb6f1d to ee46154 Compare February 29, 2024 12:43
jserv

This comment was marked as outdated.

@jserv jserv requested a review from eecheng87 February 29, 2024 13:38
linenoise.h Outdated Show resolved Hide resolved
@vax-r vax-r requested a review from jserv March 1, 2024 02:06
Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refine the subject of git commit messages: make it compliant with https://cbea.ms/git-commit/

linenoise.c Outdated Show resolved Hide resolved
@vax-r vax-r requested a review from jserv March 1, 2024 12:28
linenoise.c Outdated Show resolved Hide resolved
linenoise.c Outdated Show resolved Hide resolved
linenoise.c Outdated Show resolved Hide resolved
@jserv
Copy link
Contributor

jserv commented Mar 1, 2024

Remove "Successfully" from the subject of git commit messages. Make it informative.

@vax-r vax-r force-pushed the Integrate_linenoise_web branch 2 times, most recently from 7030a6d to fb68092 Compare March 1, 2024 12:58
@vax-r vax-r requested a review from jserv March 1, 2024 12:59
linenoise.c Outdated
@@ -927,6 +928,11 @@ static int line_edit(int stdin_fd,

if (write(l.ofd, prompt, l.plen) == -1)
return -1;

int result = web_select(l.buf, l.ifd);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proposed change lacks refinement, as it tightly integrates web server operations with the linenoise package. Linenoise is intended to be a streamlined and functional package, offering basic command line editing capabilities universally. Therefore, it's advisable to incorporate the function hook within the main loop of linenoise. This strategy will enable a standard method for callback registration, allowing web server interactions to be executed alongside linenoise's main loop, thus achieving a more decoupled approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that the main loop of linenoise is the same as the function linenoise() in linenoise.c , can I say that ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that the main loop of linenoise is the same as the function linenoise() in linenoise.c , can I say that ?

No, the main loop of linenoise package appears in line_edit function, and you check the callbacks such as completion_callback for the execution of user-defined functions. Always program in an elegant way.

@vax-r vax-r force-pushed the Integrate_linenoise_web branch 2 times, most recently from 5a0b8cf to 9783894 Compare March 1, 2024 16:17
linenoise.c Outdated Show resolved Hide resolved
@vax-r vax-r requested a review from jserv March 1, 2024 16:26
linenoise.c Outdated Show resolved Hide resolved
linenoise.c Outdated Show resolved Hide resolved
Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid using backtick in git commit message for the sake of maximum editor compatibility. You can use (double) quotation marks instead.

@vax-r vax-r force-pushed the Integrate_linenoise_web branch 2 times, most recently from 9a47445 to d1439d0 Compare March 1, 2024 17:35
@vax-r vax-r requested a review from jserv March 1, 2024 17:36
linenoise.c Outdated Show resolved Hide resolved
web.h Outdated Show resolved Hide resolved
@vax-r vax-r requested a review from jserv March 2, 2024 05:34
@@ -472,6 +473,14 @@ void line_set_free_hints_callback(line_free_hints_callback_t *fn)
free_hints_callback = fn;
}

/* Register a function to perform I/O multiplexing to monitor multiple file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improve the descriptions by explaining the functionality of this callback invoked inside the main loop of linenoise. You should discuss the use case for example.

Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refine the subject "Integrate linenoise package with web server," which is a bit misleading. In fact, web server has been integrated into the whole project, and the proposed change is not exactly referred as the integration effort. Instead, it was meant to inject the main loop of line editing by introducing an extra callback which can interoperate with the web server.

web.c Outdated Show resolved Hide resolved
In the previous version of console implementation, we tried to integrate
tiny-web-server to enable the ability of processing commands from web
requests. As the result, the package linenoise which is responsible for
command-line auto-complete needs to be disabled during the running time
of tiny-web-server. Because the main loop in "line_edit()" function in
linenoise doesn't have the ability to handle web requests correctly.

When we start the web server, we use "cmd_select" in console.c and use
"select" function to monitor web socket file descriptor and stdin_fd at
the same time. I re-design the control flow of web request and
command-line input by implement the function "web_eventmux()", and
register it as a hook function of type "line_eventmux_callback_t"
inside linenoise package. As the result, we can utilize function inside
 the main loop of linenoise which is located inside the function
"line_edit()".

"web_eventmux()" is a function which use the function "select()" to
monitor both input file descriptor and web file descriptor and modify
"line_edit()" to use "eventmux_callback()" so we can process
command-line input as normal and provide the command-line auto-complete
feature alongside with the abitlity to deal with inputs from different
input sources, such as web request.

One may wonder why don't we simply modify the function "line_edit()",
the reason is that linenoise is an upstream package so we only want to
do the miminal changes to this package.
@jserv
Copy link
Contributor

jserv commented Mar 2, 2024

Can I mention this PR in my homework-1 note ? or I should refine my commit in my personal repository and mentioned it instead ?

Go ahead. Don't go off-topic here.

@jserv jserv merged commit dd6a761 into sysprog21:master Mar 4, 2024
1 of 2 checks passed
@jserv
Copy link
Contributor

jserv commented Mar 4, 2024

Thank @vax-r for contributing!

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