Skip to content

Conversation

@ym
Copy link
Contributor

@ym ym commented Apr 7, 2025

No description provided.

@ym ym requested review from adamshiervani, chemhack and Copilot April 7, 2025 13:10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (2)

internal/usbgadget/config.go:140

  • [nitpick] Consider returning (bool, error) rather than (error, bool) to follow Go conventions and improve code readability.
func (u *UsbGadget) OverrideGadgetConfig(itemKey string, itemAttr string, value string) (error, bool) {

jsonrpc.go:488

  • [nitpick] Consider specifying the allowed modes (cdrom, disk) in the error message for clearer guidance on valid input values.
return "", fmt.Errorf("invalid mode: %s", mode)

virtualMediaStateMutex.Unlock()
return fmt.Errorf("another virtual media is already mounted")
}
setMassStorageMode(mode == CDROM)
Copy link

Copilot AI Apr 7, 2025

Choose a reason for hiding this comment

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

The error returned by setMassStorageMode is not checked, which may lead to silent failures. Consider handling the error appropriately.

Suggested change
setMassStorageMode(mode == CDROM)
err := setMassStorageMode(mode == CDROM)
if err != nil {
virtualMediaStateMutex.Unlock()
return fmt.Errorf("failed to set mass storage mode: %w", err)
}

Copilot uses AI. Check for mistakes.
Size: size,
}
virtualMediaStateMutex.Unlock()
setMassStorageMode(mode == CDROM)
Copy link

Copilot AI Apr 7, 2025

Choose a reason for hiding this comment

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

The error returned by setMassStorageMode in this call is ignored. Consider checking and handling any potential error.

Suggested change
setMassStorageMode(mode == CDROM)
err := setMassStorageMode(mode == CDROM)
if err != nil {
logger.Errorf("failed to set mass storage mode: %v", err)
return err
}

Copilot uses AI. Check for mistakes.
Comment on lines 286 to 287
setMassStorageMode(mode == CDROM)

Copy link

Copilot AI Apr 7, 2025

Choose a reason for hiding this comment

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

Ignoring the error from setMassStorageMode here can lead to unhandled failure scenarios. Please capture and handle the error as needed.

Suggested change
setMassStorageMode(mode == CDROM)
err = setMassStorageMode(mode == CDROM)
if err != nil {
return fmt.Errorf("failed to set mass storage mode: %w", err)
}

Copilot uses AI. Check for mistakes.
@ym ym force-pushed the feat/usb-mass-storage-mode branch 2 times, most recently from fe11c4d to c80945e Compare April 10, 2025 22:42
@ym ym force-pushed the feat/usb-mass-storage-mode branch from c80945e to 5ba3315 Compare May 12, 2025 16:19
@ym ym requested a review from Copilot May 12, 2025 16:19
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for mounting USB mass storage as a disk rather than a CD/DVD by renaming functions and updating the UI and RPC logic accordingly.

  • Rename backend function from getMassStorageMode() to getMassStorageCDROMEnabled().
  • Update UI components to map .img files to "Disk" mode.
  • Introduce a new gadget configuration override function in the usbgadget config.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
usb_mass_storage.go Rename function for clarity and update mass storage mode handling
ui/src/routes/devices.$id.mount.tsx Adjust UI logic to set USB mode to "Disk" for IMG files and update radio input properties
jsonrpc.go Use switch-case for mode translation and update function call to reflect renaming
internal/usbgadget/config.go Add OverrideGadgetConfig for updating gadget configurations
Comments suppressed due to low confidence (1)

usb_mass_storage.go:116

  • The renaming to 'getMassStorageCDROMEnabled' improves clarity. Please ensure related documentation or comments are updated accordingly.
func getMassStorageCDROMEnabled() (bool, error) {

Comment on lines +417 to 418
setUsbMode("Disk");
}
Copy link

Copilot AI May 12, 2025

Choose a reason for hiding this comment

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

The UI now uses 'Disk' mode for IMG files while the backend maps non-CDROM modes to 'file'. Consider aligning these terms or adding documentation to clarify this mapping for API consumers.

Suggested change
setUsbMode("Disk");
}
setUsbMode("file"); // Aligning with backend terminology
}
// Note: The UI uses "CDROM" and "file" to match backend expectations.
// "CDROM" is used for ISO files, and "file" is used for IMG files.

Copilot uses AI. Check for mistakes.
@ym ym force-pushed the feat/usb-mass-storage-mode branch from 5ba3315 to 3f8b30c Compare May 12, 2025 16:35
@ym ym force-pushed the feat/usb-mass-storage-mode branch from 7dffd13 to 023c6e2 Compare May 12, 2025 16:54
@ym ym merged commit 63c2272 into jetkvm:dev May 12, 2025
1 of 2 checks passed
ym added a commit to ym/jetkvm-kvm that referenced this pull request Sep 26, 2025
* feat(usb_mass_storage): mount as disk

* chore: try to set initial virtual media state from sysfs

* chore(usb-mass-storage): fix inquiry_string
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