[GitHub] [hadoop] goiri commented on a diff in pull request #4370: HDFS-16463. Make dirent cross platform compatible

[GitBox <gi...@apache.org>]

goiri commented on code in PR #4370:
URL: https://github.com/apache/hadoop/pull/4370#discussion_r889767207


##########
hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/lib/x-platform/c-api/dirent.cc:
##########
@@ -0,0 +1,100 @@
+/**
+ * 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.
+ */
+
+#include <algorithm>
+#include <cerrno>
+#include <iostream>
+#include <iterator>
+#include <system_error>
+#include <variant>
+
+#include "x-platform/c-api/dirent.h"
+#include "x-platform/dirent.h"
+
+#if defined(WIN32) && defined(__cplusplus)

Review Comment:
   There's no cleaner way to do this?



##########
hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/lib/x-platform/c-api/dirent.h:
##########
@@ -0,0 +1,93 @@
+/**
+ * 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.
+ */
+
+#ifndef NATIVE_LIBHDFSPP_LIB_CROSS_PLATFORM_C_API_DIRENT_H
+#define NATIVE_LIBHDFSPP_LIB_CROSS_PLATFORM_C_API_DIRENT_H
+
+/*
+ * We will use XPlatform's dirent on Windows or when the macro
+ * USE_X_PLATFORM_DIRENT is defined.
+ */
+#if defined(WIN32) || defined(USE_X_PLATFORM_DIRENT)
+
+/*
+ * We will use extern "C" only on Windows.
+ */
+#if defined(WIN32) && defined(__cplusplus)
+extern "C" {
+#endif
+
+/**
+ * DIR struct holds the pointer to XPlatform::Dirent instance. Since this will
+ * be used in C, we can't hold the pointer to XPlatform::Dirent. We're working
+ * around this by using a void pointer and casting it to XPlatform::Dirent when
+ * needed in C++.
+ */
+typedef struct DIR {
+  void *x_platform_dirent_ptr;
+} DIR;
+
+/**
+ * dirent struct contains the name of the file/folder while iterating through
+ * the directory's children.
+ */
+struct dirent {
+  char d_name[256];
+};
+
+/**
+ * Opens a directory for iteration. Internally, it instantiates DIR struct for
+ * the given path. closedir must be called on the returned pointer to DIR struct
+ * when done.
+ *
+ * @param dir_path The path to the directory to iterate through.
+ * @return A pointer to the DIR struct.
+ */
+DIR *opendir(const char *dir_path);
+
+/**
+ * For iterating through the children of the directory pointed to by the DIR
+ * struct pointer.
+ *
+ * @param dir The pointer to the DIR struct.
+ * @return A pointer to dirent struct containing the name of the current child
+ * file/folder.
+ */
+struct dirent *readdir(DIR *dir);
+
+/**
+ * De-allocates the XPlatform::Dirent instance pointed to by the DIR pointer.
+ *
+ * @param dir The pointer to DIR struct to close.
+ * @return 0 if successful.
+ */
+int closedir(DIR *dir);
+
+#if defined(WIN32) && defined(__cplusplus)
+}
+#endif
+
+#else
+/*
+ * For non-Windows environments, we use the dirent.h header itself.
+ */
+#include <dirent.h>

Review Comment:
   It might be easier to read to have this one first.



##########
hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/lib/x-platform/c-api/dirent.cc:
##########
@@ -0,0 +1,100 @@
+/**
+ * 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.
+ */
+
+#include <algorithm>
+#include <cerrno>
+#include <iostream>
+#include <iterator>
+#include <system_error>
+#include <variant>
+
+#include "x-platform/c-api/dirent.h"
+#include "x-platform/dirent.h"
+
+#if defined(WIN32) && defined(__cplusplus)
+extern "C" {
+#endif
+
+DIR *opendir(const char *dir_path) {
+  const auto dir = new DIR;
+  dir->x_platform_dirent_ptr = new XPlatform::Dirent(dir_path);
+  return dir;
+}
+
+struct dirent *readdir(DIR *dir) {
+  /*
+   * We will use a static variable to hold the dirent, so that we align with the
+   * readdir's implementation in dirent.h header file in Linux.
+   */
+  static struct dirent static_dir_entry;
+
+  // Get the XPlatform::Dirent instance and move the iterator.
+  const auto x_platform_dirent =
+      static_cast<XPlatform::Dirent *>(dir->x_platform_dirent_ptr);
+  const auto dir_entry = x_platform_dirent->NextFile();
+
+  // End of iteration.
+  if (std::holds_alternative<std::monostate>(dir_entry)) {
+    return nullptr;
+  }
+
+  // Error in iteration.
+  if (std::holds_alternative<std::error_code>(dir_entry)) {
+    const auto err = std::get<std::error_code>(dir_entry);
+    errno = err.value();
+
+#ifdef X_PLATFORM_C_API_DIRENT_DEBUG
+    std::cerr << "Error in listing directory: " << err.message() << std::endl;

Review Comment:
   Do we want to do this?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org