Patch 4/5

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Patch 4/5

Lino Sanfilippo

This patch fixes a missing cleanup in cases where an error occurs
after an event has already been assigned to a group.
Before an event has not been put back to the "todo" list properly
(the list_del() for the working list was missing) and also was not
put back in cases of error in dazukofs_group_read().

There is still something missing:
If a filedescriptor has already been opened for an event, it has to
be closed before it is put back to the "working list".
I will send a fix for this issue with another patch.


Geschäftsführender Gesellschafter: Tjark Auerbach
Sitz der Gesellschaft: Tettnang
Handelsregister: Amtsgericht Ulm, HRB 630992
ALLGEMEINE GESCHÄFTSBEDINGUNGEN
Es gelten unsere Allgemeinen Geschäftsbedingungen
(AGB). Sie finden sie in der jeweils gültigen Fassung
im Internet unter http://www.avira.de/agb
***************************************************

diff -Nurp dazukofs-3.1.3-patch3/event.c dazukofs-3.1.3-patch4/event.c
--- dazukofs-3.1.3-patch3/event.c 2010-07-06 17:28:05.000000000 +0200
+++ dazukofs-3.1.3-patch4/event.c 2010-07-08 18:10:41.000000000 +0200
@@ -28,50 +28,17 @@
 #include <linux/pid.h>
 #include <linux/slab.h>
 
+#include "event.h"
 #include "dev.h"
 #include "dazukofs_fs.h"
 
 struct dazukofs_proc {
- struct list_head list;
+ struct list_head list;
  struct pid *proc_id;
  int within_list;
 };
 
-struct dazukofs_event {
- unsigned long event_id;
- struct dentry *dentry;
- struct vfsmount *mnt;
- struct pid *proc_id;
- wait_queue_head_t queue;
-
- /* protects: deny, deprecated, assigned */
- struct mutex assigned_mutex;
-
- int deny;
- int deprecated;
- int assigned;
-};
 
-struct dazukofs_event_container {
- struct list_head list;
- struct dazukofs_event *event;
- struct file *file;
- int fd;
-};
-
-struct dazukofs_group {
- struct list_head list;
- char *name;
- size_t name_length;
- unsigned long group_id;
- struct dazukofs_event_container todo_list;
- wait_queue_head_t queue;
- struct dazukofs_event_container working_list;
- atomic_t use_count;
- int tracking;
- int track_count;
- int deprecated;
-};
 
 static struct dazukofs_group group_list;
 static int group_count;
@@ -899,6 +866,15 @@ out:
  return ret;
 }
 
+
+void set_event_pending(struct dazukofs_group *grp,
+                       struct dazukofs_event_container *ec)
+{
+ mutex_lock(&work_mutex);
+ list_add(&ec->list, &grp->working_list.list);
+ mutex_unlock(&work_mutex);
+}
+
 /**
  * unclaim_event - return an event to the todo list
  * @grp: group to which the event is assigned
@@ -907,7 +883,7 @@ out:
  * Description: This function puts the given event container on the todo
  * list and wake the group.
  */
-static void unclaim_event(struct dazukofs_group *grp,
+void unclaim_event(struct dazukofs_group *grp,
   struct dazukofs_event_container *ec)
 {
  /* put the event on the todo list */
@@ -938,7 +914,6 @@ static struct dazukofs_event_container *
  ec = list_first_entry(&grp->todo_list.list,
       struct dazukofs_event_container, list);
  list_del(&ec->list);
- list_add(&ec->list, &grp->working_list.list);
  }
  mutex_unlock(&work_mutex);
 
@@ -1034,7 +1009,6 @@ static int is_event_available(struct daz
  return ret;
 }
 
-/**
  * dazukofs_get_event - get an event to process
  * @group_id: id of the group we belong to
  * @event_id: to be filled in with the new event id
@@ -1047,8 +1021,8 @@ static int is_event_available(struct daz
  *
  * Returns 0 on success.
  */
-int dazukofs_get_event(unsigned long group_id, unsigned long *event_id,
-       int *fd, pid_t *pid)
+int dazukofs_get_event(unsigned long group_id, struct dazukofs_group **pgrp,
+                       struct dazukofs_event_container **pec)
 {
  struct dazukofs_group *grp = NULL;
  struct dazukofs_event_container *ec;
@@ -1067,10 +1041,8 @@ int dazukofs_get_event(unsigned long gro
  }
  mutex_unlock(&work_mutex);
 
- if (!found) {
- ret = -EFAULT;
- goto out;
- }
+ if (!found)
+ return -EINVAL;
 
  while (1) {
  ret = wait_event_freezable(grp->queue,
@@ -1087,14 +1059,9 @@ int dazukofs_get_event(unsigned long gro
  ec = claim_event(grp);
  if (ec) {
  ret = open_file(ec);
- if (ret == 0) {
- *event_id = ec->event->event_id;
- *fd = ec->fd;
-
- /* set to 0 if not within namespace */
- *pid = pid_vnr(ec->event->proc_id);
+ if (ret == 0)
  break;
- } else {
+ else {
  unclaim_event(grp, ec);
  if (ret == -ENFILE) {
  /* The registered process may not open
@@ -1103,8 +1070,16 @@ int dazukofs_get_event(unsigned long gro
  }
  }
  }
+ /* we did not get the event */
+ atomic_dec(&grp->use_count);
  }
- atomic_dec(&grp->use_count);
-out:
- return ret;
+
+ if (ret) {
+ atomic_dec(&grp->use_count);
+ return ret;
+ }
+
+ *pgrp = grp;
+ *pec = ec;
+ return 0;
 }
diff -Nurp dazukofs-3.1.3-patch3/event.h dazukofs-3.1.3-patch4/event.h
--- dazukofs-3.1.3-patch3/event.h 2010-07-06 17:28:05.000000000 +0200
+++ dazukofs-3.1.3-patch4/event.h 2010-07-08 16:39:46.000000000 +0200
@@ -21,11 +21,56 @@
 #ifndef __EVENT_H
 #define __EVENT_H
 
+#include <linux/list.h>
+#include <linux/wait.h>
+
+
+struct dazukofs_event {
+ unsigned long event_id;
+ struct dentry *dentry;
+ struct vfsmount *mnt;
+ struct pid *proc_id;
+ wait_queue_head_t queue;
+
+ /* protects: deny, deprecated, assigned */
+ struct mutex assigned_mutex;
+
+ int deny;
+ int deprecated;
+ int assigned;
+};
+
+struct dazukofs_event_container {
+ struct list_head list;
+ struct dazukofs_event *event;
+ struct file *file;
+ int fd;
+};
+
+struct dazukofs_group {
+ struct list_head list;
+ char *name;
+ size_t name_length;
+ unsigned long group_id;
+ struct dazukofs_event_container todo_list;
+ wait_queue_head_t queue;
+ struct dazukofs_event_container working_list;
+ atomic_t use_count;
+ int tracking;
+ int track_count;
+ int deprecated;
+};
+
 extern int dazukofs_init_events(void);
 extern void dazukofs_destroy_events(void);
 
-extern int dazukofs_get_event(unsigned long group_id,
-      unsigned long *event_id, int *fd, pid_t *pid);
+extern void set_event_pending(struct dazukofs_group *grp,
+                       struct dazukofs_event_container *ec);
+extern void unclaim_event(struct dazukofs_group *grp,
+                          struct dazukofs_event_container *ec);
+extern int dazukofs_get_event(unsigned long group_id,
+                              struct dazukofs_group **pgrp,
+      struct dazukofs_event_container **pec);
 extern int dazukofs_return_event(unsigned long group_id,
  unsigned long event_id, int deny);
 
diff -Nurp dazukofs-3.1.3-patch3/group_dev.c dazukofs-3.1.3-patch4/group_dev.c
--- dazukofs-3.1.3-patch3/group_dev.c 2010-07-06 17:40:52.000000000 +0200
+++ dazukofs-3.1.3-patch4/group_dev.c 2010-07-08 15:30:19.000000000 +0200
@@ -58,33 +58,41 @@ static ssize_t dazukofs_group_read(int g
 {
 #define DAZUKOFS_MIN_READ_BUFFER 43
  char tmp[DAZUKOFS_MIN_READ_BUFFER];
- ssize_t tmp_used;
- pid_t pid;
- int fd;
- int err;
- unsigned long event_id;
+ struct dazukofs_event_container *ec;
+ struct dazukofs_group *grp;
+ ssize_t rv;
 
 
  if (length < DAZUKOFS_MIN_READ_BUFFER)
  return -EINVAL;
 
- err = dazukofs_get_event(group_id, &event_id, &fd, &pid);
- if (err) {
+ rv = dazukofs_get_event(group_id, &grp, &ec);
+ if (rv) {
  /* convert some errors to acceptable read(2) errno values */
- if (err == -ENFILE)
- return -EIO;
- return err;
+ if (rv == -ENFILE)
+ rv = -EIO;
+ return rv;
  }
 
- tmp_used = snprintf(tmp, sizeof(tmp)-1, "id=%lu\nfd=%d\npid=%d\n",
-    event_id, fd, pid);
- if (tmp_used >= sizeof(tmp))
- return -EINVAL;
+ rv = snprintf(tmp, sizeof(tmp)-1, "id=%lu\nfd=%d\npid=%d\n",
+    ec->event->event_id, ec->fd,
+    pid_vnr(ec->event->proc_id));
+ if (rv >= sizeof(tmp)) {
+ unclaim_event(grp, ec);
+ rv = -EINVAL;
+ goto err;
+ }
 
- if (copy_to_user(buffer, tmp, tmp_used))
- return -EFAULT;
+ if (copy_to_user(buffer, tmp, rv)) {
+ unclaim_event(grp, ec);
+ rv = -EFAULT;
+ goto err;
+ }
+ set_event_pending(grp, ec);
 
- return tmp_used;
+err:
+ atomic_dec(&grp->use_count);
+ return rv;
 }
 
 static ssize_t dazukofs_group_write(int group_id, struct file *file,

_______________________________________________
Dazuko-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/dazuko-devel
Reply | Threaded
Open this post in threaded view
|

Re: Patch 4/5

John Ogness-10
On 2010-07-08, Lino Sanfilippo <[hidden email]> wrote:

> This patch fixes a missing cleanup in cases where an error occurs
> after an event has already been assigned to a group.  Before an
> event has not been put back to the "todo" list properly (the
> list_del() for the working list was missing) and also was not put
> back in cases of error in dazukofs_group_read().
>
> There is still something missing:
> If a filedescriptor has already been opened for an event, it has to
> be closed before it is put back to the "working list".  I will send
> a fix for this issue with another patch.

Thank you for pointing this out. The latest version (3.1.4-rc1) fixes
this issue and also handles closing the file descriptor. I chose not
to use your implementation because I felt it made the group use_count
handling too complicated.

John Ogness

--
Dazuko Maintainer

_______________________________________________
Dazuko-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/dazuko-devel
Reply | Threaded
Open this post in threaded view
|

Re: Patch 4/5

Lino Sanfilippo
John Ogness wrote:

> On 2010-07-08, Lino Sanfilippo <[hidden email]> wrote:
>  
>> This patch fixes a missing cleanup in cases where an error occurs
>> after an event has already been assigned to a group.  Before an
>> event has not been put back to the "todo" list properly (the
>> list_del() for the working list was missing) and also was not put
>> back in cases of error in dazukofs_group_read().
>>    
>
> Thank you for pointing this out. The latest version (3.1.4-rc1) fixes
> this issue

The issue is still there. Have a look at claim_event() where we do:
- remove item from todo list
- put item on working list

If something fails afterwards we call unclaim_event() and only do:
- put item back on todo list

Removing the item from the working list in unclaim_event() is still
missing...

Regards,
Lino



Geschäftsführender Gesellschafter: Tjark Auerbach
Sitz der Gesellschaft: Tettnang
Handelsregister: Amtsgericht Ulm, HRB 630992
ALLGEMEINE GESCHÄFTSBEDINGUNGEN
Es gelten unsere Allgemeinen Geschäftsbedingungen
(AGB). Sie finden sie in der jeweils gültigen Fassung
im Internet unter http://www.avira.com/de/standard-terms-conditions-business-de
***************************************************

_______________________________________________
Dazuko-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/dazuko-devel