You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@quickstep.apache.org by tarunbansal <gi...@git.apache.org> on 2016/09/08 04:02:05 UTC
[GitHub] incubator-quickstep pull request #99: QUICKSTEP-36 Quickstep gives segmentat...
GitHub user tarunbansal opened a pull request:
https://github.com/apache/incubator-quickstep/pull/99
QUICKSTEP-36 Quickstep gives segmentation fault when it imports table from a file that does not have access rights.
This PR fixes QUICKSTEP-36
- Perform access check on a file before opening it to import a table.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/tarunbansal/incubator-quickstep master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-quickstep/pull/99.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #99
----
commit b2d2534843edad1014520f8d7b5245350827d09f
Author: Tarun Bansal <ta...@gmail.com>
Date: 2016-09-07T04:40:28Z
QUICKSTEP-36 fixed.
Added read access check in TextScanOperator file before opening a file
commit b7f0ca1daf25fe930f4cf0bcf62ace234b880fe6
Author: tarunbansal <ta...@gmail.com>
Date: 2016-09-08T03:48:45Z
Added changes according to commiters workflow
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #99: QUICKSTEP-36 Quickstep gives segmentat...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/99#discussion_r78760515
--- Diff: relational_operators/TextScanOperator.cpp ---
@@ -19,6 +19,12 @@
#include "relational_operators/TextScanOperator.hpp"
+#include "relational_operators/RelationalOperatorsConfig.h" // For QUICKSTEP_HAVE_UNISTD.
--- End diff --
@hbdeshmukh No, this line is ok, as the next line immediately uses `QUICKSTEP_HAVE_UNISTD` defined in the header file.
See https://github.com/apache/incubator-quickstep/blob/master/storage/StorageManager.cpp#L20 for more info and examples.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #99: QUICKSTEP-36 Quickstep gives segmentation fau...
Posted by pateljm <gi...@git.apache.org>.
Github user pateljm commented on the issue:
https://github.com/apache/incubator-quickstep/pull/99
Thanks @zuyu and @hbdeshmukh for excellent mentorship on this PR!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #99: QUICKSTEP-36 Quickstep gives segmentation fau...
Posted by tarunbansal <gi...@git.apache.org>.
Github user tarunbansal commented on the issue:
https://github.com/apache/incubator-quickstep/pull/99
Hi @zuyu Thanks for the welcome and the information about coding style. I have made the suggested changes and reverted file permission change which was done erroneously in the last commit by me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #99: QUICKSTEP-36 Quickstep gives segmentat...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/99#discussion_r78439803
--- Diff: relational_operators/TextScanOperator.cpp ---
@@ -18,6 +18,11 @@
**/
#include "relational_operators/TextScanOperator.hpp"
+#include "relational_operators/RelationalConfig.h"
--- End diff --
@tarunbansal As I suggested before, please add comments `// For QUICKSTEP_HAVE_UNISTD.` after the head file.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #99: QUICKSTEP-36 Quickstep gives segmentat...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/99#discussion_r78285790
--- Diff: relational_operators/RelationalConfig.h.in ---
@@ -0,0 +1,20 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ **/
+
+#cmakedefine QUICKSTEP_HAVE_UNISTD
--- End diff --
Code style: miss the end-of-line symbol.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #99: QUICKSTEP-36 Quickstep gives segmentation fau...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/99
LGTM. Merged.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #99: QUICKSTEP-36 Quickstep gives segmentation fau...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on the issue:
https://github.com/apache/incubator-quickstep/pull/99
Hi @tarunbansal
I have given some comments. After you address them, I will merge this PR. Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #99: QUICKSTEP-36 Quickstep gives segmentat...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/99#discussion_r78285930
--- Diff: relational_operators/TextScanOperator.cpp ---
@@ -91,6 +91,15 @@ bool TextScanOperator::getAllWorkOrders(
if (blocking_dependencies_met_ && !work_generated_) {
for (const std::string &file : files) {
// Use standard C libary to retrieve the file size.
+
+ // Check file permissions before trying to open it
+ #ifdef QUICKSTEP_HAVE_UNISTD
--- End diff --
Hi @tarunbansal Welcome to the team, and please get familiar with our code style at https://cwiki.apache.org/confluence/display/QUICKSTEP/Quickstep+Coding+Standards.
Also, I noticed that you made the file mode from `644` to `755`. Please revert this change first.
Finally, I would have written the code as following:
```
// Add in Line 21.
#include "relational_operators/RelationalConfig.h" // For QUICKSTEP_HAVE_UNISTD.
#ifdef QUICKSTEP_HAVE_UNISTD
#include <unistd.h>
#endif // QUICKSTEP_HAVE_UNISTD
// Start from Line 95.
#ifdef QUICKSTEP_HAVE_UNISTD
// Check file permissions before trying to open it.
const int access_result = access(file.c_str(), R_OK);
CHECK_EQ(0, access_result)
<< "File " << file << " is not readable due to permission issues.";
#endif // QUICKSTEP_HAVE_UNISTD
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #99: QUICKSTEP-36 Quickstep gives segmentat...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-quickstep/pull/99
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #99: QUICKSTEP-36 Quickstep gives segmentat...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/99#discussion_r78439950
--- Diff: relational_operators/TextScanOperator.cpp ---
@@ -91,6 +96,14 @@ bool TextScanOperator::getAllWorkOrders(
if (blocking_dependencies_met_ && !work_generated_) {
for (const std::string &file : files) {
// Use standard C libary to retrieve the file size.
+
+#ifdef QUICKSTEP_HAVE_UNISTD
+ // Check file permissions before trying to open it.
+ const int access_result = access(file.c_str(), R_OK);
+ CHECK_EQ(0, access_result)
+ << "File " << file << " is not readable due to permission issues.";
--- End diff --
Please remove one of two whitespaces before `file`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #99: QUICKSTEP-36 Quickstep gives segmentat...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/99#discussion_r78382769
--- Diff: relational_operators/TextScanOperator.cpp ---
@@ -18,6 +18,11 @@
**/
#include "relational_operators/TextScanOperator.hpp"
+#include "relational_operators/RelationalConfig.h"
--- End diff --
Can you sort the above two included files alphabetically? We follow a convention that lists of included files are sorted. If you are dealing with a large list of included files, you may use your IDE's sorting feature, instead of manual effort.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #99: QUICKSTEP-36 Quickstep gives segmentat...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/99#discussion_r78382304
--- Diff: relational_operators/RelationalConfig.h.in ---
@@ -0,0 +1,20 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ **/
+
+#cmakedefine QUICKSTEP_HAVE_UNISTD
--- End diff --
To be consistent with the similar files in other modules, can we rename this file to ``RelationalOperatorsConfig.h.in``?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #99: QUICKSTEP-36 Quickstep gives segmentat...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/99#discussion_r77946830
--- Diff: relational_operators/TextScanOperator.cpp ---
@@ -91,6 +91,12 @@ bool TextScanOperator::getAllWorkOrders(
if (blocking_dependencies_met_ && !work_generated_) {
for (const std::string &file : files) {
// Use standard C libary to retrieve the file size.
+
+ // Check file permissions before trying to open it
+ int rval = access(file.c_str(), R_OK);
--- End diff --
Hi @tarunbansal - This is a neat change.
The ``access`` function may not be available on all the platforms that are used in Quickstep. On unix, it comes from the header ``unistd.h``.
To solve such issues, typically we check the availability of this header file using CMake. If the function is available, we create a variable, e.g. in this case it could be QUICKSTEP_HAVE_UNISTD and guard this line with ``#ifdef QUICKSTEP_HAVE_UNISTD`` and ``#endif QUICKSTEP_HAVE_UNISTD``. You can dig around in the code to find such usage patterns.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #99: QUICKSTEP-36 Quickstep gives segmentat...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/99#discussion_r78755999
--- Diff: relational_operators/TextScanOperator.cpp ---
@@ -19,6 +19,12 @@
#include "relational_operators/TextScanOperator.hpp"
+#include "relational_operators/RelationalOperatorsConfig.h" // For QUICKSTEP_HAVE_UNISTD.
--- End diff --
Can you move this include between line 39 and 50, i.e. in the block of included files from quickstep modules. When you do that, make sure the list is sorted.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #99: QUICKSTEP-36 Quickstep gives segmentat...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/99#discussion_r78760896
--- Diff: relational_operators/TextScanOperator.cpp ---
@@ -19,6 +19,12 @@
#include "relational_operators/TextScanOperator.hpp"
+#include "relational_operators/RelationalOperatorsConfig.h" // For QUICKSTEP_HAVE_UNISTD.
--- End diff --
Ah, good catch. Thanks @zuyu
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #99: QUICKSTEP-36 Quickstep gives segmentation fau...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/99
@tarunbansal Before we merge this PR, please squash all the commits into one. Thanks!
https://cwiki.apache.org/confluence/display/QUICKSTEP/Workflow+For+Contributors
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #99: QUICKSTEP-36 Quickstep gives segmentation fau...
Posted by tarunbansal <gi...@git.apache.org>.
Github user tarunbansal commented on the issue:
https://github.com/apache/incubator-quickstep/pull/99
Thanks @hbdeshmukh and @zuyu for the suggestions and help. I have squashed all the commits into one. Please merge this PR if it is good to go now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #99: QUICKSTEP-36 Quickstep gives segmentat...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/99#discussion_r78439566
--- Diff: relational_operators/TextScanOperator.cpp ---
@@ -18,6 +18,11 @@
**/
#include "relational_operators/TextScanOperator.hpp"
+#include "relational_operators/RelationalConfig.h"
--- End diff --
@hbdeshmukh Alternatively, I suggest to add an empty line between Line 20 and 21.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #99: QUICKSTEP-36 Quickstep gives segmentation fau...
Posted by tarunbansal <gi...@git.apache.org>.
Github user tarunbansal commented on the issue:
https://github.com/apache/incubator-quickstep/pull/99
Thanks @hbdeshmukh for noticing this possible ambiguity. I am still digging around the code and will update the PR once the fix is ready.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---