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

add-optional-permissions: add ability to change permissions of screen recording file and path #475

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alailsonko
Copy link

Description

Add record-path-permissions and record-file-permissions optional fields when starting record of session.

@alailsonko alailsonko changed the base branch from master to staging/1.5.5 January 6, 2024 19:54
@alailsonko alailsonko changed the base branch from staging/1.5.5 to master January 6, 2024 19:56
@necouchman
Copy link
Contributor

necouchman commented Jan 6, 2024

@alailsonko: Thanks for the pull request and contribution to the project. You'll need to get a Jira account and create a Jira issue for this, and then tag both the pull request and the commit messages with the Jira issue (GUACAMOLE-XXXX:) before we'll accept and merge this.

Please see the following page for full contribution guidelines: https://guacamole.apache.org/open-source/

Comment on lines +167 to +168
const char* path, const char* name, int create_path, mode_t file_permissions,
mode_t path_permissions, int include_output, int include_mouse, int include_touch,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about directly coupling the API for screen recordings to POSIX-specific mode_t when libguac is generally intended to support Windows as a platform. Perhaps there's a way to implement this without making guac_recording_create() non-portable?

If it were possible to implement this in a way that doesn't require API/ABI-breaking changes, that would also be ideal.

Copy link
Author

Choose a reason for hiding this comment

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

i dont have the jira account for create issues at the moment.

thanks for letting me know and giving attention to this PR, i understand it is very delicate change.

we could change from mode_t to int, i think it would be agnostic. or creating own mode_t type.

i was reading documentation of c in microsoft and they use type int for mode.

int _open(
   const char *filename,
   int oflag [,
   int pmode]
);
int _wopen(
   const wchar_t *filename,
   int oflag [,
   int pmode]
);
errno_t _sopen_s(
   int* pfh,
   const char *filename,
   int oflag,
   int shflag,
   int pmode
);
errno_t _wsopen_s(
   int* pfh,
   const wchar_t *filename,
   int oflag,
   int shflag,
   int pmode,
);

@alailsonko alailsonko marked this pull request as draft January 7, 2024 20:54
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