[kudu-CR] env posix: add another failure case to Walk

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8171 )

Change subject: env_posix: add another failure case to Walk
..


Patch Set 1: Code-Review+2

(1 comment)

After some consideration I think that handling FTS_DNR might be considered as 
an optional thing, please free to ignore.

http://gerrit.cloudera.org:8080/#/c/8171/1/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:

http://gerrit.cloudera.org:8080/#/c/8171/1/src/kudu/util/env_posix.cc@1475
PS1, Line 1475: case FTS_NS
> Well, I would try to create a directory X then 'chmod  X'.  I think tha
Yep, it worked as expected to me on OS X:

$ mkdir /tmp/test
$ chmod  /tmp/test
$ clang++ -o x x.cc
$ ./x
FTS_D
FTS_DNR: unable to access file /tmp/test during walk: Permission denied


x.cc contained the following sketchy code:

#include 
#include 
#include 

#include 

using namespace std;

int main() {
  char* paths[] = { "/tmp/test", nullptr };
  FTS* tree = fts_open(paths, FTS_PHYSICAL | FTS_XDEV | FTS_NOCHDIR, nullptr);
  if (!tree) {
cerr << "fts_open() failed" << endl;
return 1;
  }

  while (true) {
FTSENT* ent = fts_read(tree);
if (!ent) {
  break;
}

switch (ent->fts_info) {
  case FTS_D: // Directory in pre-order
cout << "FTS_D" << endl;
break;
  case FTS_DP:// Directory in post-order
cout << "FTS_DP" << endl;
break;
  case FTS_F: // A regular file
  case FTS_SL:// A symbolic link
  case FTS_SLNONE:// A broken symbolic link
  case FTS_DEFAULT:   // Unknown type of file
cout << "FTS_F|FTS_SL|FTS_SLNONE|FTS_DEFAULT" << endl;
break;

  case FTS_ERR:
cerr << "FTS_ERR: unable to access file " << ent->fts_path
 << " during walk: " << strerror(ent->fts_errno) << endl;
  case FTS_NS:
cerr << "FTS_NS: unable to access file " << ent->fts_path
 << " during walk: " << strerror(ent->fts_errno) << endl;
  case FTS_DNR:
cerr << "FTS_DNR: unable to access file " << ent->fts_path
 << " during walk: " << strerror(ent->fts_errno) << endl;
break;

  default:
cerr << "Unable to access file " << ent->fts_path
 << " during walk (code " << ent->fts_info << ")" << endl;
break;
}
  }
  fts_close(tree);

  return 0;
}



--
To view, visit http://gerrit.cloudera.org:8080/8171
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I16edaec169d074913768c95a16173ff105d7b6a1
Gerrit-Change-Number: 8171
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 29 Sep 2017 05:46:15 +
Gerrit-HasComments: Yes

[kudu-CR] env posix: add another failure case to Walk

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8171 )

Change subject: env_posix: add another failure case to Walk
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8171/1/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:

http://gerrit.cloudera.org:8080/#/c/8171/1/src/kudu/util/env_posix.cc@1475
PS1, Line 1475: case FTS_NS
> I could do that, but how would you test for it?
Well, I would try to create a directory X then 'chmod  X'.  I think that 
would do it -- need to test that.



--
To view, visit http://gerrit.cloudera.org:8080/8171
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I16edaec169d074913768c95a16173ff105d7b6a1
Gerrit-Change-Number: 8171
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 29 Sep 2017 03:23:09 +
Gerrit-HasComments: Yes

[kudu-CR] env posix: add another failure case to Walk

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8171 )

Change subject: env_posix: add another failure case to Walk
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8171/1/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:

http://gerrit.cloudera.org:8080/#/c/8171/1/src/kudu/util/env_posix.cc@1475
PS1, Line 1475: case FTS_NS
> From reading 'man fts_read' it seems the FTS_DNR should also be treated as
I could do that, but how would you test for it?



--
To view, visit http://gerrit.cloudera.org:8080/8171
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I16edaec169d074913768c95a16173ff105d7b6a1
Gerrit-Change-Number: 8171
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 29 Sep 2017 02:58:29 +
Gerrit-HasComments: Yes

[kudu-CR] env posix: add another failure case to Walk

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8171 )

Change subject: env_posix: add another failure case to Walk
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8171/1/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:

http://gerrit.cloudera.org:8080/#/c/8171/1/src/kudu/util/env_posix.cc@1475
PS1, Line 1475: case FTS_NS
>From reading 'man fts_read' it seems the FTS_DNR should also be treated as 
>error where fts_errno is set.

Would it make sense to add that case here as well?



--
To view, visit http://gerrit.cloudera.org:8080/8171
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I16edaec169d074913768c95a16173ff105d7b6a1
Gerrit-Change-Number: 8171
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 29 Sep 2017 02:29:54 +
Gerrit-HasComments: Yes

[kudu-CR] env posix: add another failure case to Walk

[Adar Dembo (Code Review)]

Hello Alexey Serbin, Todd Lipcon,

I'd like you to do a code review. Please visit

http://gerrit.cloudera.org:8080/8171

to review the following change.


Change subject: env_posix: add another failure case to Walk
..

env_posix: add another failure case to Walk

The manpage for fts_read has this to say about FTS_NS:

  A file for which no stat(2) information was available.   The  con‐
  tents  of  the  fts_statp  field  are undefined.  This is an error
  return, and the fts_errno field  will  be  set  to  indicate  what
  caused the error.

As such, we should treat it the same as FTS_ERR.

The new test fails without this change, which I found somewhat surprising;
I would have expected fts_open(2) to at least check for the existence of the
root directory, but apparently that's deferred to fts_read(2).

Change-Id: I16edaec169d074913768c95a16173ff105d7b6a1
---
M src/kudu/util/env-test.cc
M src/kudu/util/env_posix.cc
2 files changed, 38 insertions(+), 33 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/8171/1
--
To view, visit http://gerrit.cloudera.org:8080/8171
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I16edaec169d074913768c95a16173ff105d7b6a1
Gerrit-Change-Number: 8171
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Todd Lipcon