Support serving pxelinux artifacts #6
Conversation
Potentially makes netboot support pxelinux machines that directly ignore the pxe server and go for the tftpserver, thus not sending too much info other that a pxelinux path and client config This would make it support those clients with minimal changes to other parts and allow to configure the served files Signed-off-by: Itxaka <itxaka@kairos.io>
|
not tested because.... well, no idea what has pxelinux support this days? |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Copilot
AI
left a comment
Copilot
AI
left a comment
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 suggestions.
Comments skipped due to low confidence (2)
server/tftp.go:92
- [nitpick] The variable 'i' is used to store both integer and string values. It should be renamed to something more descriptive for clarity.
_, i, err := extractInfoPxeLinux(path)
server/tftp.go:121
- The extractInfoPxeLinux function should be covered by tests to ensure its correctness.
func extractInfoPxeLinux(path string) (net.HardwareAddr, string, error) {
| return nil, "", errors.New("not found") | ||
| } | ||
| cleanedMac := strings.Replace(pathElements[1], "01-", "", 1) | ||
| mac, _ = net.ParseMAC(cleanedMac) |
There was a problem hiding this comment.
The error from net.ParseMAC is ignored. It should be handled properly to avoid potential issues with invalid MAC addresses.
| mac, _ = net.ParseMAC(cleanedMac) | |
| mac, err := net.ParseMAC(cleanedMac); if err != nil { return nil, "", err } |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
There was a problem hiding this comment.
nope, because we may not get a mac in here, and we want to try but dont care if we fail.
| if strings.HasPrefix(path, "pxelinux.cfg/") { | ||
| _, i, err := extractInfoPxeLinux(path) | ||
| if err != nil { | ||
| return nil, 0, fmt.Errorf("unknown path %q", path) |
There was a problem hiding this comment.
The error message "unknown path %q" is used for both extractInfoPxeLinux and extractInfo. This could be confusing. Differentiate the error messages for clarity.
| return nil, 0, fmt.Errorf("unknown path %q", path) | |
| return nil, 0, fmt.Errorf("unknown pxelinux path %q", path) |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
then why add support for it? |
It's easy, and potentially covers future use of the lib or consumers to support it? I know that s390x machines only support pxelinux booting so.... Maybe? |
Potentially makes netboot support pxelinux machines that directly ignore the pxe server and go for the tftpserver, thus not sending too much info other that a pxelinux path and client config
This would make it support those clients with minimal changes to other parts and allow to configure the served files