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

Vla/static buffer removal #2405

Closed
wants to merge 7 commits into from
Closed

Vla/static buffer removal #2405

wants to merge 7 commits into from

Conversation

Wirtos
Copy link
Contributor

@Wirtos Wirtos commented Jun 19, 2021

No description provided.

@rom1v
Copy link
Collaborator

rom1v commented Jun 19, 2021

Thank you. But since it's just local, IMO it's overkill to compute the length then alloc, you could just alloc 8192 directly (and it is freed immediately).

@Wirtos
Copy link
Contributor Author

Wirtos commented Jun 19, 2021

Thank you. But since it's just local, IMO it's overkill to compute the length then alloc, you could just alloc 8192 directly (and it is freed immediately).

Thought about it too. What about the length of additional args? Should I account for them too?

rom1v pushed a commit that referenced this pull request Jun 19, 2021
This is more explicit.

PR #2405 <#2405>

Signed-off-by: Romain Vimont <[email protected]>
rom1v pushed a commit that referenced this pull request Jun 19, 2021
And increase the command buffer size.

Refs #1358 <#1358 (comment)>
PR #2405 <#2405>

Signed-off-by: Romain Vimont <[email protected]>
@rom1v
Copy link
Collaborator

rom1v commented Jun 19, 2021

Thank you very much.

I made the following changes:

  • remove the reverted commits
  • rename cmd to argv in a separate commit (for diff clarity)
  • remove CMD_MAX from process.h and used separate local constants for windows implementation and error message
  • simplify the logic in sys/win/process.c

I pushed to branch novla. Please review the result to tell me if this is ok for you.

Regards

@Wirtos
Copy link
Contributor Author

Wirtos commented Jun 20, 2021

Seems fine, not entirely sure why len here

#define MAX_COMMAND_STRING_LEN 1024
differs from here
#define CMD_MAX_LEN 8192

since they're both being fed the same argv?

@@ -32,6 +31,8 @@

#endif

#define CMD_MAX 8192
Copy link
Collaborator

Choose a reason for hiding this comment

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

The CMD_MAX limit in scrcpy is specific to Windows (on Linux, each argument is passed separately), so it should not be in the shared process.h.

@@ -6,6 +6,11 @@
#include "util/log.h"
#include "util/str_util.h"

#if !defined(S_ISREG) && defined(S_IFMT) && defined(S_IFREG)
#define S_ISREG(m) (((m) & S_IFMT) == S_IFREG)
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

off-topic

switch (err) {
case PROCESS_ERROR_GENERIC:
argv_to_string(argv, buf, sizeof(buf));
argv_to_string(argv, buf, CMD_MAX);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The limit has no reason to equal CMD_MAX here (the string also contains other characters).

Since it's for an error message (and the argv_to_string() message explicitly truncates the command, I think it is ok to limit to a smaller value (typically, the error is in argv[0] due to SCRCPY_SERVER_PATH anyway, that's the reason why I added the log).

@rom1v
Copy link
Collaborator

rom1v commented Jun 20, 2021

Seems fine, not entirely sure why len here […]

Oh, sorry, my inline reviews were not committed on github (still pending). The reason is explained here: #2405 (comment)
I guess 512 would be sufficient, but it's arbitrary (In practice, it will not make any difference.)

rom1v pushed a commit that referenced this pull request Jun 20, 2021
This is more explicit.

PR #2405 <#2405>

Signed-off-by: Romain Vimont <[email protected]>
rom1v pushed a commit that referenced this pull request Jun 20, 2021
And increase the command buffer size.

Refs #1358 <#1358 (comment)>
PR #2405 <#2405>

Signed-off-by: Romain Vimont <[email protected]>
@rom1v
Copy link
Collaborator

rom1v commented Jun 20, 2021

Merged. 👍

@rom1v rom1v closed this Jun 20, 2021
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