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

[qemu-usermode] Simplify safe syscall patches. #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

krnlyng
Copy link
Contributor

@krnlyng krnlyng commented Jan 12, 2023

After sailfishos/scratchbox2#21
is merged, we can now let syscalls go through the C syscall
function.

DO NOT MERGE BEFORE:
sailfishos/scratchbox2#21

#define QEMU_H

-#include "hostdep.h"
+// #include "hostdep.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this required?

Copy link
Contributor

Choose a reason for hiding this comment

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

In case it is just remove that line. I would make sense to explain this better as this change applies to any use of qemu since it not only applies to sb2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it's required, because it'll redirect the syscall to an assembly implementation otherwise and sb2 can't handle that. The explanation is in the spec file.

Copy link
Member

@vigejolla vigejolla Jan 13, 2023

Choose a reason for hiding this comment

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

Generally speaking, I'm a firm believer of the "there shouldn't be commented code" rule. But I think this just might be the exception. But I also think Thaodan has a point - not everyone is going to go read the spec file to find the explanation here. Replacing this with // #include "hostdep.h" commented out because sb2 handles syscalls going through c interface or something like that would make it clear. Bit this is all just nitpicking really.

@Thaodan
Copy link
Contributor

Thaodan commented Jan 13, 2023 via email

@vigejolla
Copy link
Member

LGTM, except for the WIP in title, and the nitpick about the commented out include

After sailfishos/scratchbox2#21
is merged, we can now let syscalls go through the C syscall
function.
@krnlyng krnlyng changed the title WIP: [qemu-usermode] Simplify safe syscall patches. [qemu-usermode] Simplify safe syscall patches. Aug 22, 2023
@krnlyng krnlyng changed the title [qemu-usermode] Simplify safe syscall patches. [qemu-usermode] Simplify safe syscall patches. Aug 22, 2023
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.

3 participants