-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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 flags when open file #7445
Add flags when open file #7445
Conversation
94d066a
to
72e0a14
Compare
Hi, it seems the clang_tidy check run win32 implementation under linux platform. And I am not sure how to fix that.
|
@scw00 thanks for the contribution. Can you describe the purpose of this change? AFAICT the new functionality is not used? Are you planning on a follow up change? |
Hi @mattklein123 . I am trying to implement the static file filter which return static file from local fs. But I didn't found any appropriate structure for reading a file. It is the first step to implement the opening method with reading only flag. |
6c4d30f
to
f86d9b4
Compare
Updated. For using std::bitset and set default value to open method. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, a few high level comments to get started. Please also check clang-tidy and format.
/wait
45d2bc7
to
80bdf02
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks for working on this. A few small comments otherwise LGTM.
/wait
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with some nits, thanks.
/wait
@@ -12,6 +12,10 @@ | |||
namespace Envoy { | |||
namespace Filesystem { | |||
|
|||
static constexpr FlagSet DEFAULT_FLAGS{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the default flags function that is already defined, same elsewhere (I already commented on this I think).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I can use the default function which is defined in access_log_manager_impl.h
. file_system_test
should not dependent on other implementations .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM. @jmarantz any further comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry I should've chimed in earlier; I do have some questions.
@@ -83,6 +83,9 @@ class AccessLogFileImpl : public AccessLogFile { | |||
void open(); | |||
void createFlushStructures(); | |||
|
|||
// return default flags set which used by open | |||
Filesystem::FlagSet defaultFlags(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Declare this method static; it does not appear to require this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks and will do.
|
||
fd_ = ::open(path_.c_str(), flags, mode); | ||
void FileImplPosix::openFile(FlagSet in) { | ||
const auto flags_and_perm = translateFlag(in); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You reference 'perm' in this PR a few times; can we spell that out? I think it might mean "permanent" but I'm not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean move this into shared interface . It seems contradictory to previous changes, see
#7445 (comment)
This is also why I make the PermAndFlags
into platform implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh it's hard to see exactly what Matt had an issue with because you squashed commits. In general please try to avoid that because we lose the comment history.
I still don't understand the meaning of 'perm'. But looking at how it's used here I retract my "make part of the interface" suggestion, and instead suggest using the correct type and name in posix for the 3rd arg to open: mode_t mode
@@ -16,8 +16,14 @@ class FileImplPosix : public FileSharedImpl { | |||
~FileImplPosix() override; | |||
|
|||
protected: | |||
struct FlagsAndPerm { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to put FlagsAndPerm in a shared interface, rather than repeating it from win32 and posix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the translateFlags method is platform specified function ( the translateFlags
is in the platform class not in shared interface). So the implementation should have their own flags and perm or other something more than this two flags.
But currently we only care about the perm and flags, so I'm ok with that . And will change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/perm/mode/ and s/int/mode_t/ for the mode arg?
fd_ = ::_open(path_.c_str(), flags, mode); | ||
void FileImplWin32::openFile(FlagSet in) { | ||
const auto flags_and_perm = translateFlag(in); | ||
fd_ = ::open(path_.c_str(), flags_and_perm.flags, flags_and_perm.perm); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah I finally figured out what 'perm' meant by reading the MS docs: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/open-wopen?view=vs-2019 -- it means "permissions".
but there it's called pmode
, not perm
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks and will change
@@ -14,8 +14,14 @@ class FileImplWin32 : public FileSharedImpl { | |||
~FileImplWin32(); | |||
|
|||
protected: | |||
struct FlagsAndPerm { | |||
int flags = 0; | |||
int perm = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pmode
@@ -16,8 +16,14 @@ class FileImplPosix : public FileSharedImpl { | |||
~FileImplPosix() override; | |||
|
|||
protected: | |||
struct FlagsAndMode { | |||
int flags = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: end member variables with _
Signed-off-by: cwsong <cwsong@iflytek.com>
/retest |
🤷♀️ nothing to rebuild. |
/azp run envoy-macos |
Azure Pipelines successfully started running 1 pipeline(s). |
Thanks @dio |
Hi @mattklein123 @jmarantz thanks for your comments. And Can I get another approve ? |
Thanks |
Add flags for open method. So user can use it with read only mode or write only mode.
[Optional Fixes #Issue] no issue
[Optional Deprecated:]