Add device module and tests with default nop functions for open/release#232
Add device module and tests with default nop functions for open/release#232Vrohs wants to merge 1 commit intoluainkernel:masterfrom
Conversation
|
|
||
| local device = require("device") | ||
| local socket = require("socket") | ||
| local linux = require("linux") |
There was a problem hiding this comment.
please don't do cosmetic changes such this along with other changes.. instead, if you would like to propose new formatting or variable naming, please, submit it as a separate PR..
There was a problem hiding this comment.
please mind all cosmetic changes
There was a problem hiding this comment.
I found this overly complex, I would instead use the same approach we used on socket, that is, having a higher-level library to introduce these default values.. it could be something as simpler as..
local chardev = require("device.char")
local driver = chardev.new{...}
and
local device = require("device")
local chardev = {}
local function nop() end
function chardev.new(t)
t.open = t.open or nop
...
return device.new(t)
end
return chardev
|
Thank you for the feedback! I've made the following changes:
The implementation is now much simpler and follows the pattern used in the socket module. |
| @@ -0,0 +1,30 @@ | |||
| -- | |||
| -- SPDX-FileCopyrightText: (c) 2023-2025 Ring Zero Desenvolvimento de Software LTDA | |||
There was a problem hiding this comment.
you might use your own copyright on new files (as long as it's also new code)
| local device = require("device") | ||
| local chardev = {} | ||
|
|
||
| -- No-operation function to use as default for open and release |
There was a problem hiding this comment.
please, avoid unnecessary comments like this..
| -- No-operation function to use as default for open and release | ||
| local function nop() end | ||
|
|
||
| -- Create a new character device with default values for open and release |
|
|
||
| -- Create a new character device with default values for open and release | ||
| function chardev.new(t) | ||
| -- If t is a string, convert it to a table with name field |
| t = {name = t} | ||
| end | ||
|
|
||
| -- Ensure t is a table |
| -- Ensure t is a table | ||
| t = t or {} | ||
|
|
||
| -- Set default values for open and release if not provided |
There was a problem hiding this comment.
why only open and release? perhaps it would be better to have a table with all file operations..
| t.open = t.open or nop | ||
| t.release = t.release or nop | ||
|
|
||
| -- Call the original device.new function |
There was a problem hiding this comment.
please avoid this kind of comment
| return device.new(t) | ||
| end | ||
|
|
||
| return chardev |
There was a problem hiding this comment.
we are using \n at the end..
|
|
||
| local s = linux.stat | ||
| local tap = {name = "tap", open = nop, release = nop, mode = s.IRUGO} | ||
| local tap = {name = "tap", mode = s.IRUGO} |
There was a problem hiding this comment.
perhaps a good idea is to also have a default mode
| device.new("patch_test2") | ||
|
|
||
| print("Patch test script loaded successfully") | ||
| print("Check /dev/patch_test1 and /dev/patch_test2") |
There was a problem hiding this comment.
I don't think this file is necessary
|
|
||
| print("Device module patched: device.new() now has nop as default for open and release") | ||
|
|
||
| return device |
There was a problem hiding this comment.
I don't think this file is necessary..
|
|
||
| device.new(test3) | ||
|
|
||
| print("Device test script loaded successfully") |
There was a problem hiding this comment.
I don't think this file is necessary
lneto
left a comment
lneto
left a comment
There was a problem hiding this comment.
please review all unnecessary code and squash your commits.. let's make the history as cleaner as possible..
btw, thank you for contributing!
|
I've addressed all the feedback:
The code is now much cleaner and more concise. |
you didn't go through all comments.. e.g.,
I would also add documentation and adjust the code base to use this lib (not only tap), including the /dev/passwd example in the doc.. |
|
looked into the mentioned feedback. I'm really sorry about bothering too much, I couldn't find clear guidelines about contributing to this project. |
I think you missed my point.. we shouldn't rename things or change formatting just for cosmetic sake.. it's still plenty of places you are doing that.. we don't do this along with other changes to have a better track of the source history. Moreover, I don't mean to create a passwd example, but to update the one in the top of README. I think you still need to go through my comments and handle all of them.. you don't need to do swiftly; it's better to take the time to handle it properly, make sure you understand them all, asking questions if something is not clear, etc.. so we avoid back and forth..
it's not bothering.. just regular code review =).. unfortunately we don't have a guideline, but you are welcome to help building one. P.S.: thank for your contribution again =) |
|
@Vrohs are you still planning to make the changes I've pointed out? |
Yes I'm working on it. |
lneto
force-pushed
the
master
branch
2 times, most recently
from
17ac113 to
925bfd9
Compare
2 participants
This pull request introduces a new device module and updates several scripts to incorporate the new functionality. The most important changes include adding default behaviors to the device module, creating new test scripts, and updating the Makefile to include the new device scripts.
Enhancements to the device module:
lib/device.lua: Enhanced the device module by providing default values foropenandreleasefunctions if they are not specified. This ensures that the device module is more robust and easier to use.lib/device/core.lua: Added a stub module that forwards to the C implementation of the device module.New test scripts:
examples/device_test.lua: Created a test script to demonstrate the creation of devices with various configurations, including using default values foropenandreleasefunctions.examples/patch_device.lua: Added a script to patch thedevice.newfunction to setnopas the default foropenandreleasefunctions, ensuring backward compatibility and ease of use.examples/patch_test.lua: Created a test script to verify the patcheddevice.newfunction, ensuring it works correctly with the new defaults.Makefile updates:
Makefile: Updated thescripts_installandscripts_uninstalltargets to include the new device scripts, ensuring they are properly installed and uninstalled.