You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/06/14 07:14:56 UTC
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3901: nuttx: Initialize chmod\fchmod\chown\fchown\lchown
patacongo commented on a change in pull request #3901:
URL: https://github.com/apache/incubator-nuttx/pull/3901#discussion_r649987151
##########
File path: fs/vfs/fs_chmod.c
##########
@@ -0,0 +1,103 @@
+/****************************************************************************
+ * fs/vfs/fs_chmod.c
+ *
+ * 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.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <sys/types.h>
+#include <sys/stat.h>
+
+#include <errno.h>
+#include <assert.h>
+
+#include <nuttx/fs/fs.h>
+
+#include "inode/inode.h"
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: chown
+ *
+ * Description:
+ * changes the mode of the file specified whose pathname is given in
+ * pathname, which is dereferenced if it is a symbolic link.
+ *
+ * NOTE: Due to the characteristics of fs, this function is incomplete.
+ *
+ * Input Parameters:
+ * fd : fd.
+ * mode : Permission value.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success; -1 is returned on fail, and errno is
+ * set appropriately.
+ *
+ ****************************************************************************/
+
+int chmod(FAR const char *path, mode_t mode)
+{
+ if (path == NULL)
+ {
+ return -EINVAL;
+ }
+
+#ifdef CONFIG_FILE_MODE
+ struct inode_search_s desc;
+ FAR struct inode *inode;
+ int ret;
+
+ /* Get an inode for this file */
+
+ SETUP_SEARCH(&desc, path, false);
+
+ ret = inode_find(&desc);
+ if (ret < 0)
+ {
+ /* "O_CREAT is not set and the named file does not exist. Or, a
+ * directory component in pathname does not exist or is a dangling
+ * symbolic link."
+ */
Review comment:
Remove reference to O_CREAT
##########
File path: fs/vfs/fs_chmod.c
##########
@@ -0,0 +1,103 @@
+/****************************************************************************
+ * fs/vfs/fs_chmod.c
+ *
+ * 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.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <sys/types.h>
+#include <sys/stat.h>
+
+#include <errno.h>
+#include <assert.h>
+
+#include <nuttx/fs/fs.h>
+
+#include "inode/inode.h"
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: chown
+ *
+ * Description:
+ * changes the mode of the file specified whose pathname is given in
+ * pathname, which is dereferenced if it is a symbolic link.
+ *
+ * NOTE: Due to the characteristics of fs, this function is incomplete.
+ *
+ * Input Parameters:
+ * fd : fd.
+ * mode : Permission value.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success; -1 is returned on fail, and errno is
+ * set appropriately.
+ *
+ ****************************************************************************/
+
+int chmod(FAR const char *path, mode_t mode)
+{
+ if (path == NULL)
+ {
+ return -EINVAL;
+ }
+
+#ifdef CONFIG_FILE_MODE
+ struct inode_search_s desc;
+ FAR struct inode *inode;
+ int ret;
+
+ /* Get an inode for this file */
+
+ SETUP_SEARCH(&desc, path, false);
+
+ ret = inode_find(&desc);
+ if (ret < 0)
+ {
+ /* "O_CREAT is not set and the named file does not exist. Or, a
+ * directory component in pathname does not exist or is a dangling
+ * symbolic link."
+ */
+
+ RELEASE_SEARCH(&desc);
+ set_errno(ret);
+ return ERROR;
+ }
+
+ /* Get the search results */
+
+ inode = desc.node;
+ DEBUGASSERT(inode != NULL);
+
+ inode->i_mode = mode;
+
Review comment:
Shouldn't the mode be restricted for a read-only file system? Would be an error to set O_WRONLY on a read-only file system?
##########
File path: fs/vfs/fs_fchmod.c
##########
@@ -0,0 +1,83 @@
+/****************************************************************************
+ * fs/vfs/fs_fchmod.c
+ *
+ * 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.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <sys/types.h>
+#include <sys/stat.h>
+
+#include <errno.h>
+#include <assert.h>
+
+#include <nuttx/fs/fs.h>
+
+#include "inode/inode.h"
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: chown
+ *
+ * Description:
+ * changes the mode of the file referred to by the open file descriptor fd.
+ *
+ * NOTE: Due to the characteristics of fs, this function is incomplete.
+ *
+ * Input Parameters:
+ * fd : fd.
+ * mode : Permission value.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success; -1 is returned on fail, and errno is
+ * set appropriately.
+ *
+ ****************************************************************************/
+
+int fchmod(int fd, mode_t mode)
+{
+#ifdef CONFIG_FILE_MODE
+ FAR struct file *filep;
+ FAR struct inode *inode;
+ ssize_t ret;
+
+ /* First, get the file structure.
+ * Note that fs_getfilep() will return the errno on failure.
+ */
+
+ ret = (ssize_t)fs_getfilep(fd, &filep);
+ if (ret >= 0)
+ {
+ inode = filep->f_inode;
+ DEBUGASSERT(inode != NULL);
+
+ inode->i_mode = mode;
+ return OK;
Review comment:
So comment with chmod()
##########
File path: fs/vfs/fs_chown.c
##########
@@ -0,0 +1,105 @@
+/****************************************************************************
+ * fs/vfs/fs_chown.c
+ *
+ * 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.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <sys/types.h>
+#include <sys/stat.h>
+
+#include <errno.h>
+#include <unistd.h>
+#include <pwd.h>
+#include <grp.h>
+#include <assert.h>
+
+#include <nuttx/fs/fs.h>
+
+#include "inode/inode.h"
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: chown
+ *
+ * Description:
+ * changes the ownership of the file specified by pathname, which is
+ * dereferenced if it is a symbolic link.
+ *
+ * NOTE: Due to the characteristics of fs, this function is incomplete.s
+ *
+ * Input Parameters:
+ * pathname: path of file.
+ * owner : User id.
+ * group : Group id.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success; -1 is returned on fail, and errno is
+ * set appropriately.
+ *
+ ****************************************************************************/
+
+int chown(FAR const char *pathname, uid_t owner, gid_t group)
+{
+ struct inode_search_s desc;
+ FAR struct inode *inode;
+ int ret;
+
+ if (pathname == NULL || (owner <= -1 && group <= -1))
+ {
+ set_errno(-EINVAL);
+ return ERROR;
+ }
+ /* Get an inode for this file */
+
+ SETUP_SEARCH(&desc, pathname, false);
+
+ ret = inode_find(&desc);
+ if (ret < 0)
+ {
+ /* "O_CREAT is not set and the named file does not exist. Or, a
+ * directory component in pathname does not exist or is a dangling
+ * symbolic link."
+ */
+
+ RELEASE_SEARCH(&desc);
+ set_errno(ret);
+ return ERROR;
+ }
+
+ /* Get the search results */
+
+ inode = desc.node;
+ DEBUGASSERT(inode != NULL);
+
+ /* Set uid and gid */
+
+ inode->i_uid = owner > -1 ? owner : inode->i_uid;
+ inode->i_gid = group > -1 ? group : inode->i_gid;
+
Review comment:
NSH supports an /etc/passwd file in apps/nshlib/nsh_passwdcmds.c and a library is available in apps/fsutils/passwd. It would be nice to support /etc/user and /etc/group and use these to verify that the owner and group are valid.
We also need to set the uid and gid of the task in struct task_group_s. NSH supports a login using apps/fsutils/passwd and could setuid and setgid. That would be the other piece of supporting a proper security model.
So it would also be good if we could assure that the caller has the privileges to modify the mode, uid, and gid.
##########
File path: include/nuttx/fs/fs.h
##########
@@ -352,6 +352,8 @@ struct inode
int16_t i_crefs; /* References to inode */
uint16_t i_flags; /* Flags for inode */
union inode_ops_u u; /* Inode operations */
+ uid_t i_uid; /* The ID of the inode owner */
+ gid_t i_gid; /* The ID of the inode owner group */
#ifdef CONFIG_FILE_MODE
Review comment:
Is the initial value zero? That is the root user and group. Shouldn't the initial value be the user and group ID of the task creating the inode?
##########
File path: fs/vfs/fs_fchmod.c
##########
@@ -0,0 +1,83 @@
+/****************************************************************************
+ * fs/vfs/fs_fchmod.c
+ *
+ * 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.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <sys/types.h>
+#include <sys/stat.h>
+
+#include <errno.h>
+#include <assert.h>
+
+#include <nuttx/fs/fs.h>
+
+#include "inode/inode.h"
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: chown
+ *
+ * Description:
+ * changes the mode of the file referred to by the open file descriptor fd.
+ *
+ * NOTE: Due to the characteristics of fs, this function is incomplete.
+ *
+ * Input Parameters:
+ * fd : fd.
+ * mode : Permission value.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success; -1 is returned on fail, and errno is
+ * set appropriately.
+ *
+ ****************************************************************************/
+
+int fchmod(int fd, mode_t mode)
+{
+#ifdef CONFIG_FILE_MODE
+ FAR struct file *filep;
+ FAR struct inode *inode;
+ ssize_t ret;
+
+ /* First, get the file structure.
+ * Note that fs_getfilep() will return the errno on failure.
+ */
+
+ ret = (ssize_t)fs_getfilep(fd, &filep);
+ if (ret >= 0)
+ {
+ inode = filep->f_inode;
+ DEBUGASSERT(inode != NULL);
+
+ inode->i_mode = mode;
+ return OK;
Review comment:
Same comments as with chmod()
##########
File path: fs/vfs/fs_chmod.c
##########
@@ -0,0 +1,103 @@
+/****************************************************************************
+ * fs/vfs/fs_chmod.c
+ *
+ * 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.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <sys/types.h>
+#include <sys/stat.h>
+
+#include <errno.h>
+#include <assert.h>
+
+#include <nuttx/fs/fs.h>
+
+#include "inode/inode.h"
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: chown
+ *
+ * Description:
+ * changes the mode of the file specified whose pathname is given in
+ * pathname, which is dereferenced if it is a symbolic link.
+ *
+ * NOTE: Due to the characteristics of fs, this function is incomplete.
+ *
+ * Input Parameters:
+ * fd : fd.
+ * mode : Permission value.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success; -1 is returned on fail, and errno is
+ * set appropriately.
+ *
+ ****************************************************************************/
+
+int chmod(FAR const char *path, mode_t mode)
+{
+ if (path == NULL)
+ {
+ return -EINVAL;
+ }
+
+#ifdef CONFIG_FILE_MODE
+ struct inode_search_s desc;
+ FAR struct inode *inode;
+ int ret;
+
+ /* Get an inode for this file */
+
+ SETUP_SEARCH(&desc, path, false);
+
+ ret = inode_find(&desc);
+ if (ret < 0)
+ {
+ /* "O_CREAT is not set and the named file does not exist. Or, a
+ * directory component in pathname does not exist or is a dangling
+ * symbolic link."
+ */
+
+ RELEASE_SEARCH(&desc);
+ set_errno(ret);
+ return ERROR;
+ }
+
+ /* Get the search results */
+
+ inode = desc.node;
+ DEBUGASSERT(inode != NULL);
+
+ inode->i_mode = mode;
+
Review comment:
>
>
> I don't quite understand what you mean. Do you mean that the permission of the file is read-only?
Hmm... I don't think my comment applies to the pseudo file system. In a true file, the _struct file_operations_ or _struct mountpt_operation_ will have the write method set to NULL on read only file system. For example, https://github.com/patacongo/incubator-nuttx/blob/master/fs/romfs/fs_romfs.c#L102
inodes in the pseudo file system on not generally read-able or write-able, but may refer to a read-able or write-able entity such as a device driver or a file (via a mount point). In those case, I would think that the mode would be handled in a different way. The file mode is selected for the file or driver via the _open()_ call (or later via _fcntl()).
Shouldn't _chmod()_ work just like _fcntl()_
##########
File path: fs/vfs/fs_chmod.c
##########
@@ -0,0 +1,103 @@
+/****************************************************************************
+ * fs/vfs/fs_chmod.c
+ *
+ * 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.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <sys/types.h>
+#include <sys/stat.h>
+
+#include <errno.h>
+#include <assert.h>
+
+#include <nuttx/fs/fs.h>
+
+#include "inode/inode.h"
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: chown
+ *
+ * Description:
+ * changes the mode of the file specified whose pathname is given in
+ * pathname, which is dereferenced if it is a symbolic link.
+ *
+ * NOTE: Due to the characteristics of fs, this function is incomplete.
+ *
+ * Input Parameters:
+ * fd : fd.
+ * mode : Permission value.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success; -1 is returned on fail, and errno is
+ * set appropriately.
+ *
+ ****************************************************************************/
+
+int chmod(FAR const char *path, mode_t mode)
+{
+ if (path == NULL)
+ {
+ return -EINVAL;
+ }
+
+#ifdef CONFIG_FILE_MODE
+ struct inode_search_s desc;
+ FAR struct inode *inode;
+ int ret;
+
+ /* Get an inode for this file */
+
+ SETUP_SEARCH(&desc, path, false);
+
+ ret = inode_find(&desc);
+ if (ret < 0)
+ {
+ /* "O_CREAT is not set and the named file does not exist. Or, a
+ * directory component in pathname does not exist or is a dangling
+ * symbolic link."
+ */
+
+ RELEASE_SEARCH(&desc);
+ set_errno(ret);
+ return ERROR;
+ }
+
+ /* Get the search results */
+
+ inode = desc.node;
+ DEBUGASSERT(inode != NULL);
+
+ inode->i_mode = mode;
+
Review comment:
>
>
> I don't quite understand what you mean. Do you mean that the permission of the file is read-only?
Hmm... I don't think my comment applies to the pseudo file system. In a true file, the _struct file_operations_ or _struct mountpt_operation_ will have the write method set to NULL on read only file system. For example, https://github.com/patacongo/incubator-nuttx/blob/master/fs/romfs/fs_romfs.c#L102
inodes in the pseudo file system on not generally read-able or write-able, but may refer to a read-able or write-able entity such as a device driver or a file (via a mount point). In those case, I would think that the mode would be handled in a different way. The file mode is selected for the file or driver via the _open()_ call (or later via _fcntl()).
Shouldn't _chmod()_ work just like _fcntl()_:
https://github.com/patacongo/incubator-nuttx/blob/master/fs/vfs/fs_fcntl.c#L135
https://github.com/patacongo/incubator-nuttx/blob/master/fs/vfs/fs_fcntl.c#L163
Shouldn't _chmod()_, _setuid()_, and _setgid()_ just be wrappers around _fcntl()_ for real files. I guess I am just having trouble understanding what these APIs mean for the pseudo file system.
##########
File path: fs/vfs/fs_chmod.c
##########
@@ -0,0 +1,103 @@
+/****************************************************************************
+ * fs/vfs/fs_chmod.c
+ *
+ * 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.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <sys/types.h>
+#include <sys/stat.h>
+
+#include <errno.h>
+#include <assert.h>
+
+#include <nuttx/fs/fs.h>
+
+#include "inode/inode.h"
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: chown
+ *
+ * Description:
+ * changes the mode of the file specified whose pathname is given in
+ * pathname, which is dereferenced if it is a symbolic link.
+ *
+ * NOTE: Due to the characteristics of fs, this function is incomplete.
+ *
+ * Input Parameters:
+ * fd : fd.
+ * mode : Permission value.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success; -1 is returned on fail, and errno is
+ * set appropriately.
+ *
+ ****************************************************************************/
+
+int chmod(FAR const char *path, mode_t mode)
+{
+ if (path == NULL)
+ {
+ return -EINVAL;
+ }
+
+#ifdef CONFIG_FILE_MODE
+ struct inode_search_s desc;
+ FAR struct inode *inode;
+ int ret;
+
+ /* Get an inode for this file */
+
+ SETUP_SEARCH(&desc, path, false);
+
+ ret = inode_find(&desc);
+ if (ret < 0)
+ {
+ /* "O_CREAT is not set and the named file does not exist. Or, a
+ * directory component in pathname does not exist or is a dangling
+ * symbolic link."
+ */
+
+ RELEASE_SEARCH(&desc);
+ set_errno(ret);
+ return ERROR;
+ }
+
+ /* Get the search results */
+
+ inode = desc.node;
+ DEBUGASSERT(inode != NULL);
+
+ inode->i_mode = mode;
+
Review comment:
>
>
> > I don't quite understand what you mean. Do you mean that the permission of the file is read-only?
>
> Hmm... I don't think my comment applies to the pseudo file system. In a true file, the _struct file_operations_ or _struct mountpt_operation_ will have the write method set to NULL on read only file system. For example, https://github.com/patacongo/incubator-nuttx/blob/master/fs/romfs/fs_romfs.c#L102
>
> inodes in the pseudo file system on not generally read-able or write-able, but may refer to a read-able or write-able entity such as a device driver or a file (via a mount point). In those case, I would think that the mode would be handled in a different way. The file mode is selected for the file or driver via the _open()_ call (or later via _fcntl()).
>
> Shouldn't _chmod()_ work just like _fcntl()_:
>
> https://github.com/patacongo/incubator-nuttx/blob/master/fs/vfs/fs_fcntl.c#L135
> https://github.com/patacongo/incubator-nuttx/blob/master/fs/vfs/fs_fcntl.c#L163
>
> Shouldn't _chmod()_ just be a wrappers around _fcntl()_ for real files. I guess I am just having trouble understanding what these APIs mean for the pseudo file system.
##########
File path: fs/vfs/fs_chmod.c
##########
@@ -0,0 +1,103 @@
+/****************************************************************************
+ * fs/vfs/fs_chmod.c
+ *
+ * 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.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <sys/types.h>
+#include <sys/stat.h>
+
+#include <errno.h>
+#include <assert.h>
+
+#include <nuttx/fs/fs.h>
+
+#include "inode/inode.h"
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: chown
+ *
+ * Description:
+ * changes the mode of the file specified whose pathname is given in
+ * pathname, which is dereferenced if it is a symbolic link.
+ *
+ * NOTE: Due to the characteristics of fs, this function is incomplete.
+ *
+ * Input Parameters:
+ * fd : fd.
+ * mode : Permission value.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success; -1 is returned on fail, and errno is
+ * set appropriately.
+ *
+ ****************************************************************************/
+
+int chmod(FAR const char *path, mode_t mode)
+{
+ if (path == NULL)
+ {
+ return -EINVAL;
+ }
+
+#ifdef CONFIG_FILE_MODE
+ struct inode_search_s desc;
+ FAR struct inode *inode;
+ int ret;
+
+ /* Get an inode for this file */
+
+ SETUP_SEARCH(&desc, path, false);
+
+ ret = inode_find(&desc);
+ if (ret < 0)
+ {
+ /* "O_CREAT is not set and the named file does not exist. Or, a
+ * directory component in pathname does not exist or is a dangling
+ * symbolic link."
+ */
+
+ RELEASE_SEARCH(&desc);
+ set_errno(ret);
+ return ERROR;
+ }
+
+ /* Get the search results */
+
+ inode = desc.node;
+ DEBUGASSERT(inode != NULL);
+
+ inode->i_mode = mode;
+
Review comment:
>
>
> I don't quite understand what you mean. Do you mean that the permission of the file is read-only?
Hmm... I don't think my comment applies to the pseudo file system. In a true file, the _struct file_operations_ or _struct mountpt_operation_ will have the write method set to NULL on read only file system. For example, https://github.com/patacongo/incubator-nuttx/blob/master/fs/romfs/fs_romfs.c#L102
inodes in the pseudo file system on not generally read-able or write-able, but may refer to a read-able or write-able entity such as a device driver or a file (via a mount point). In those case, I would think that the mode would be handled in a different way. The file mode is selected for the file or driver via the _open()_ call (or later via _fcntl()).
Shouldn't _chmod()_ work just like _fcntl()_:
https://github.com/patacongo/incubator-nuttx/blob/master/fs/vfs/fs_fcntl.c#L135
https://github.com/patacongo/incubator-nuttx/blob/master/fs/vfs/fs_fcntl.c#L163
Shouldn't _chmod()_ just be a wrappersaround _fcntl()_ for real files and device drivers. I guess I am just having trouble understanding what these APIs mean for the pseudo file system.
##########
File path: fs/vfs/fs_chown.c
##########
@@ -0,0 +1,105 @@
+/****************************************************************************
+ * fs/vfs/fs_chown.c
+ *
+ * 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.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <sys/types.h>
+#include <sys/stat.h>
+
+#include <errno.h>
+#include <unistd.h>
+#include <pwd.h>
+#include <grp.h>
+#include <assert.h>
+
+#include <nuttx/fs/fs.h>
+
+#include "inode/inode.h"
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: chown
+ *
+ * Description:
+ * changes the ownership of the file specified by pathname, which is
+ * dereferenced if it is a symbolic link.
+ *
+ * NOTE: Due to the characteristics of fs, this function is incomplete.s
+ *
+ * Input Parameters:
+ * pathname: path of file.
+ * owner : User id.
+ * group : Group id.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success; -1 is returned on fail, and errno is
+ * set appropriately.
+ *
+ ****************************************************************************/
+
+int chown(FAR const char *pathname, uid_t owner, gid_t group)
+{
+ struct inode_search_s desc;
+ FAR struct inode *inode;
+ int ret;
+
+ if (pathname == NULL || (owner <= -1 && group <= -1))
+ {
+ set_errno(-EINVAL);
+ return ERROR;
+ }
+ /* Get an inode for this file */
+
+ SETUP_SEARCH(&desc, pathname, false);
+
+ ret = inode_find(&desc);
+ if (ret < 0)
+ {
+ /* "O_CREAT is not set and the named file does not exist. Or, a
+ * directory component in pathname does not exist or is a dangling
+ * symbolic link."
+ */
+
+ RELEASE_SEARCH(&desc);
+ set_errno(ret);
+ return ERROR;
+ }
+
+ /* Get the search results */
+
+ inode = desc.node;
+ DEBUGASSERT(inode != NULL);
+
+ /* Set uid and gid */
+
+ inode->i_uid = owner > -1 ? owner : inode->i_uid;
+ inode->i_gid = group > -1 ? group : inode->i_gid;
+
Review comment:
Thinking more, I don't think there should be i_uid or i_gid in the inode structure:
1. The user and group IDs are assign to a task
2. Entries in a file system do not have UIDs or GIDs. Instead, they have priveleges based on UID and GID, for example, _-rwxrw-r--_
##########
File path: fs/vfs/fs_lchown.c
##########
@@ -0,0 +1,106 @@
+/****************************************************************************
+ * fs/vfs/fs_lchown.c
+ *
+ * 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.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <sys/types.h>
+#include <sys/stat.h>
+
+#include <errno.h>
+#include <unistd.h>
+#include <pwd.h>
+#include <grp.h>
+#include <assert.h>
+
+#include <nuttx/fs/fs.h>
+
+#include "inode/inode.h"
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: lchown
+ *
+ * Description:
+ * changes the ownership of the file specified by pathname, but does not
+ * dereference symbolic links.
+ *
+ * NOTE: Due to the characteristics of fs, this function is incomplete.
+ *
+ * Input Parameters:
+ * pathname: path of file.
+ * owner : User id.
+ * group : Group id.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success; -1 is returned on fail, and errno is
+ * set appropriately.
+ *
+ ****************************************************************************/
+
+int lchown(FAR const char *pathname, uid_t owner, gid_t group)
+{
+ struct inode_search_s desc;
+ FAR struct inode *inode;
+ int ret;
+
+ if (pathname == NULL || (owner <= -1 && group <= -1))
+ {
+ set_errno(-EINVAL);
+ return ERROR;
+ }
+
+ /* Get an inode for this file */
+
+ SETUP_SEARCH(&desc, pathname, true);
+
+ ret = inode_find(&desc);
+ if (ret < 0)
+ {
+ /* "O_CREAT is not set and the named file does not exist. Or, a
+ * directory component in pathname does not exist or is a dangling
+ * symbolic link."
+ */
+
+ RELEASE_SEARCH(&desc);
+ set_errno(ret);
+ return ERROR;
+ }
+
+ /* Get the search results */
+
+ inode = desc.node;
+ DEBUGASSERT(inode != NULL);
+
+ /* Set uid and gid */
+
+ inode->i_uid = owner > -1 ? owner : inode->i_uid;
+ inode->i_gid = group > -1 ? group : inode->i_gid;
+
Review comment:
Same comments as for _chown()_
##########
File path: fs/vfs/fs_chmod.c
##########
@@ -0,0 +1,103 @@
+/****************************************************************************
+ * fs/vfs/fs_chmod.c
+ *
+ * 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.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <sys/types.h>
+#include <sys/stat.h>
+
+#include <errno.h>
+#include <assert.h>
+
+#include <nuttx/fs/fs.h>
+
+#include "inode/inode.h"
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: chown
+ *
+ * Description:
+ * changes the mode of the file specified whose pathname is given in
+ * pathname, which is dereferenced if it is a symbolic link.
+ *
+ * NOTE: Due to the characteristics of fs, this function is incomplete.
+ *
+ * Input Parameters:
+ * fd : fd.
+ * mode : Permission value.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success; -1 is returned on fail, and errno is
+ * set appropriately.
+ *
+ ****************************************************************************/
+
+int chmod(FAR const char *path, mode_t mode)
+{
+ if (path == NULL)
+ {
+ return -EINVAL;
+ }
+
+#ifdef CONFIG_FILE_MODE
+ struct inode_search_s desc;
+ FAR struct inode *inode;
+ int ret;
+
+ /* Get an inode for this file */
+
+ SETUP_SEARCH(&desc, path, false);
+
+ ret = inode_find(&desc);
+ if (ret < 0)
+ {
+ /* "O_CREAT is not set and the named file does not exist. Or, a
+ * directory component in pathname does not exist or is a dangling
+ * symbolic link."
+ */
+
+ RELEASE_SEARCH(&desc);
+ set_errno(ret);
+ return ERROR;
+ }
+
+ /* Get the search results */
+
+ inode = desc.node;
+ DEBUGASSERT(inode != NULL);
+
+ inode->i_mode = mode;
+
Review comment:
>
>
> I don't quite understand what you mean. Do you mean that the permission of the file is read-only?
Hmm... I don't think my comment applies to the pseudo file system. In a true file, the _struct file_operations_ or _struct mountpt_operation_ will have the write method set to NULL on read only file system. For example, https://github.com/patacongo/incubator-nuttx/blob/master/fs/romfs/fs_romfs.c#L102
inodes in the pseudo file system on not generally read-able or write-able, but may refer to a read-able or write-able entity such as a device driver or a file (via a mount point). In those case, I would think that the mode would be handled in a different way. The file mode is selected for the file or driver via the _open()_ call (or later via _fcntl()).
Shouldn't _chmod()_ work just like _fcntl()_:
https://github.com/patacongo/incubator-nuttx/blob/master/fs/vfs/fs_fcntl.c#L135
https://github.com/patacongo/incubator-nuttx/blob/master/fs/vfs/fs_fcntl.c#L163
Shouldn't _chmod()_ just be a wrapper around _fcntl()_ for real files and device drivers. I guess I am just having trouble understanding what these APIs mean for the pseudo file system.
Hmm.. except that _fcntl()_ only supports a subset of mode bits.
##########
File path: fs/vfs/fs_chown.c
##########
@@ -0,0 +1,105 @@
+/****************************************************************************
+ * fs/vfs/fs_chown.c
+ *
+ * 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.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <sys/types.h>
+#include <sys/stat.h>
+
+#include <errno.h>
+#include <unistd.h>
+#include <pwd.h>
+#include <grp.h>
+#include <assert.h>
+
+#include <nuttx/fs/fs.h>
+
+#include "inode/inode.h"
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: chown
+ *
+ * Description:
+ * changes the ownership of the file specified by pathname, which is
+ * dereferenced if it is a symbolic link.
+ *
+ * NOTE: Due to the characteristics of fs, this function is incomplete.s
+ *
+ * Input Parameters:
+ * pathname: path of file.
+ * owner : User id.
+ * group : Group id.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success; -1 is returned on fail, and errno is
+ * set appropriately.
+ *
+ ****************************************************************************/
+
+int chown(FAR const char *pathname, uid_t owner, gid_t group)
+{
+ struct inode_search_s desc;
+ FAR struct inode *inode;
+ int ret;
+
+ if (pathname == NULL || (owner <= -1 && group <= -1))
+ {
+ set_errno(-EINVAL);
+ return ERROR;
+ }
+ /* Get an inode for this file */
+
+ SETUP_SEARCH(&desc, pathname, false);
+
+ ret = inode_find(&desc);
+ if (ret < 0)
+ {
+ /* "O_CREAT is not set and the named file does not exist. Or, a
+ * directory component in pathname does not exist or is a dangling
+ * symbolic link."
+ */
+
+ RELEASE_SEARCH(&desc);
+ set_errno(ret);
+ return ERROR;
+ }
+
+ /* Get the search results */
+
+ inode = desc.node;
+ DEBUGASSERT(inode != NULL);
+
+ /* Set uid and gid */
+
+ inode->i_uid = owner > -1 ? owner : inode->i_uid;
+ inode->i_gid = group > -1 ? group : inode->i_gid;
+
Review comment:
Thinking more, I don't think there should be i_uid or i_gid in the inode structure:
1. The user and group IDs are assign to a task
2. Entries in a file system do not have UIDs or GIDs. Instead, they have priveleges based on UID and GID, for example, _-rwxrw-r--_
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org