HikariKnight / quickpassthrough

A project to remove the complexity of setting up GPU passthrough for qemu
MIT License
707 stars 21 forks source link

Feat: conditional permissions behavior #28

Closed yunginnanet closed 3 months ago

yunginnanet commented 4 months ago

Overview

Current behavior asks for sudo password via stdin regardless on if we're root or not.

This PR adds an IsRoot boolean to the configuration struct and adjusts the final message appropriately, as well as other changes listed below.

Change Items

HikariKnight commented 4 months ago

I also noticed that if we tell people that they can run quickpassthrough itself as root now, we will have to check if the config and backup folders are owned by root and tell them to re-run as root if quickpassthrough is not run through sudo.

I originally designed it to be run as the normal user so i never actually tested running it as sudo then normal user đŸ˜…

other than that i like the changes so far :smile:

yunginnanet commented 4 months ago

Thanks for the feedback! I was actually just checking in to ping you to request comment, so I'm happy to see you replied! I'll take a look :)

yunginnanet commented 4 months ago

@HikariKnight take a look when you can, I believe I've addressed your comments

but this is a pretty heavy refactor so I definitely need a second pair of eyes

yunginnanet commented 4 months ago

The behavior that actually lead to me starting on this was that I would actually receive silent errors when I ran this command as a user after running it as root. This last commit, https://github.com/HikariKnight/quickpassthrough/pull/28/commits/395f5ab6bca280e06031480270d50ad0afe942d8, finalizes my major changes for now I believe.

Here's a demo of the behavior the aforementioned commit replaces silent fatal errors with:

Fatal error demonstration

HikariKnight commented 4 months ago

this gif looks nice, conveniently i am sick today so i surprisingly has freetime to look at the code once i have the paperwork done

HikariKnight commented 4 months ago

This looks good, will do a test build locally later today and test in VMs to make sure things work properly in ubuntu, pop, arch and fedora since this is a big refactor.

yunginnanet commented 4 months ago

This looks good, will do a test build locally later today and test in VMs to make sure things work properly in ubuntu, pop, arch and fedora since this is a big refactor.

Yes, this definitely needs to be looked at carefully and tested. Thanks for your efforts, and take care of yourself.

yunginnanet commented 4 months ago

^ The code should be easier to review now, as well.

HikariKnight commented 4 months ago

Tested and working on Fedora Workstation (Grubby and Dracut test)

HikariKnight commented 4 months ago

Archlinux tested and works (pure grub and mkinitcpio test)

only need the pop os test to succeed then we should be good @yunginnanet

yunginnanet commented 4 months ago

agreed on all points. adapter function added, complete with unit test