Hi Guennadi,
On Wed, 14 Apr 2010 18:14:37 +0200 (CEST)
Guennadi Liakhovetski <g.liakhovetski@xxxxxx> wrote:
> Looks good in general, some comments below:
Thanks for review and comments, please see my reply below.
> On Fri, 9 Apr 2010, Anatolij Gustschin wrote:
>
> > This patch extends PTP gadget to support DeleteObject
> > operation.
> >
> > Signed-off-by: Anatolij Gustschin <agust@xxxxxxx>
> > ---
> > ptp.c | 206 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > 1 files changed, 203 insertions(+), 3 deletions(-)
> >
> > diff --git a/ptp.c b/ptp.c
> > index b887176..87adbd4 100644
> > --- a/ptp.c
> > +++ b/ptp.c
...
> > if (unlink(thumb)) {
> > + fprintf(stderr, "Cannot delete %s: %s\n",
> > + thumb, strerror(errno));
> > + }
>
> Please, remove superfluous braces.
Done.
...
> > +static int delete_file(const char *name,
> > + enum pima15740_response_code *resp_code)
>
> I don't like this very much. delete_file() returns an int, which is
> ignored by callers, and the actual return code is passed as a parameter.
> Please, either make it void or return the response code.
You are right, I've changed to return response code.
> > +{
> > + struct stat st;
> > + int ret;
> > + uid_t euid;
> > + gid_t egid;
> > +
> > + /* access() is unreliable on NFS, we use stat() instead */
> > + ret = stat(name, &st);
> > + if (ret < 0) {
> > + fprintf(stderr, "Cannot stat %s: %s\n", name, strerror(errno));
> > + *resp_code = PIMA15740_RESP_GENERAL_ERROR;
> > + return -1;
> > + }
> > +
> > + euid = geteuid();
> > + if (euid == st.st_uid) {
> > + if (!(st.st_mode & S_IWUSR)) {
> > + *resp_code = PIMA15740_RESP_OBJECT_WRITE_PROTECTED;
> > + return -1;
> > + }
> > + goto del;
> > + }
> > +
> > + egid = getegid();
> > + if (egid == st.st_gid) {
> > + if (!(st.st_mode & S_IWGRP)) {
> > + *resp_code = PIMA15740_RESP_OBJECT_WRITE_PROTECTED;
> > + return -1;
> > + }
> > + goto del;
> > + }
> > +
> > + if (!(st.st_mode & S_IWOTH)) {
> > + *resp_code = PIMA15740_RESP_OBJECT_WRITE_PROTECTED;
> > + return -1;
> > + }
>
> Isn't there an easier and less error-prone way to verify? Maybe just try
> to delete, if errno == EPERM - return write-protected? Any specific reason
> for doing it this way? Would this also cover SELinux, ACL,... Notice, I
> have no idea about either of them, please, explain.
Trying to delete using unlink() will remove the file. But we may not
delete read-only files and must return Object_WriteProtected response
code in such cases. Therefore I'm checking for effective uid/gid write
permissions of the file.
> > +
> > +del:
> > + ret = unlink(name);
> > + if (ret) {
> > + fprintf(stderr, "Cannot delete %s: %s\n",
> > + name, strerror(errno));
> > + *resp_code = PIMA15740_RESP_GENERAL_ERROR;
>
> If you accept my proposal to just try to unlink, you'd check for EACCESS /
> EPERM here, and then set PIMA15740_RESP_OBJECT_WRITE_PROTECTED response
> code, otherwise PIMA15740_RESP_GENERAL_ERROR.
No, this won't work. EPERM is set if the file system doesn't allow
unlinking of files. EACCESS is set if write access to the directory
containing the file is not allowed for the process's effective UID, or
if one of the directories in the path to the file didn't allow search
permission. If we want to allow deletion of files in the image directory
neither of these conditions must be true, anyway. unlink() successfully
deletes the file even if this file doesn't grant write permissions.
unlink() is allowed to delete the file even if it is owned by another
user. Having write permission to the directory containing the file is
enough to delete the file.
> > + return -1;
> > + }
> > +
> > + *resp_code = PIMA15740_RESP_OK;
> > + return 0;
> > +}
> > +
> > +static int update_free_space(void)
> > +{
> > + unsigned long long bytes;
> > + struct statfs fs;
> > + int ret;
> > +
> > + ret = statfs(root, &fs);
> > + if (ret < 0) {
> > + fprintf(stderr, "statfs %s: %s\n", root, strerror(errno));
> > + return ret;
> > + }
> > +
> > + if (verbose > 1)
> > + fprintf(stdout, "Block-size %d, total %d, free %d\n",
> > + fs.f_bsize, (int)fs.f_blocks, (int)fs.f_bfree);
> > +
> > + bytes = (unsigned long long)fs.f_bsize * fs.f_bfree;
> > + storage_info.free_space_in_bytes = __cpu_to_le64(bytes);
> > + return 0;
> > +}
> > +
> > +static void delete_object(void *recv_buf, void *send_buf)
> > +{
> > + struct ptp_container *r_container = recv_buf;
> > + struct ptp_container *s_container = send_buf;
> > + enum pima15740_response_code code = PIMA15740_RESP_OK;
> > + uint32_t format, handle;
> > + uint32_t *param;
> > + unsigned long length;
> > + int ret = 0;
> > +
> > + length = __le32_to_cpu(r_container->length);
> > +
> > + param = (uint32_t *)r_container->payload;
> > + handle = __le32_to_cpu(*param);
> > + format = __le32_to_cpu(*(param + 1));
> > +
> > + if (length > 16 && format != PTP_PARAM_UNUSED) {
>
> I think, you can allow format == PTP_PARAM_ANY at no additional cost - you
> anyway support deletion of all objects, and they are all images so far...
No, subsequent patches add support for text file objects, so not
all objects will be images. We currently do not support specification
by ObjectFormatCode and PIMA15740 spec requires returning
Specification_By_Format_Unsupported error code in this case.
...
> > + if (handle == PTP_PARAM_ANY) {
> > + struct obj_list *obj, **anchor;
> > + int partial = 0;
> > +
> > + anchor = &images;
> > + obj = images;
> > +
> > + if (!obj) {
> > + code = PIMA15740_RESP_INVALID_OBJECT_HANDLE;
>
> Shall we not return in this case - we have been requested to delete all
> objects, and there are none. This looks like a success to me;) Then you
> wouldn't have to special case this at all.
Okay, changed to return PIMA15740_RESP_OK in this case.
> > + goto resp;
> > + }
> > +
> > + while (obj) {
> > + delete_file(obj->name, &code);
> > + if (code == PIMA15740_RESP_OK) {
> > + delete_thumb(obj);
> > + *anchor = obj->next;
> > + free(obj);
> > + obj = *anchor;
> > + object_number--;
> > + } else {
> > + anchor = &obj->next;
> > + obj = obj->next;
> > + partial++;
> > + }
> > + }
> > +
> > + if (partial)
> > + code = PIMA15740_RESP_PARTIAL_DELETION;
> > +
>
> Superfluous empty line
Removed.
...
> > + if (obj) {
> > + delete_file(obj->name, &code);
> > + if (code == PIMA15740_RESP_OK) {
> > + delete_thumb(obj);
> > + *anchor = obj->next;
> > + free(obj);
> > + object_number--;
> > + }
> > + } else
> > + code = PIMA15740_RESP_INVALID_OBJECT_HANDLE;
>
> Missing "{}"
Fixed.
> > +
>
> Superfluous empty line
Fixed in next patch version.
Thanks,
Anatolij
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
