Re: [systemd-devel] [PATCHv2 1/2] udev: export tags of dead device nodes to /run/udev/static_node-tags/

2013-07-16 Thread Tom Gundersen 
On Sun, Jul 14, 2013 at 2:51 PM, Dave Reisner d...@falconindy.com wrote:
 @@ -152,9 +153,9 @@ enum token_type {
  TK_A_OWNER_ID,  /* uid_t */
  TK_A_GROUP_ID,  /* gid_t */
  TK_A_MODE_ID,   /* mode_t */
 +TK_A_TAG,   /* val */
  TK_A_STATIC_NODE,   /* val */
  TK_A_ENV,   /* val, attr */
 -TK_A_TAG,   /* val */

 Is this swap really needed?

Yeah, this indicates the precedence of the tokens and we need to
handle the TAG tokens before the STATIC_NODE ones.

 @@ -2532,18 +2540,56 @@ void udev_rules_apply_static_dev_perms(struct 
 udev_rules *rules)
  case TK_A_MODE_ID:
  mode = cur-key.mode;
  break;
 +case TK_A_TAG:
 +t = strv_append(tags, rules_str(rules, 
 cur-key.value_off));

 Isn't strv_extend what you want here? It wouldn't copy the whole string
 array, just push the string onto it.

Indeed, fixed.

 +finish:
 +if (f) {
 +fflush(f);
 +fchmod(fileno(f), 0644);
 +if (ferror(f) || rename(path, /run/udev/static_node-tags) 
  0) {
 +r = -errno;

 I'm not sure this will capture a useful/accurate errno if fflush()
 fails, setting the error flag on the FILE*.

As discussed on IRC, this should be fine.

 diff --git a/src/udev/udevd.c b/src/udev/udevd.c
 index c4127cd..285f9a0 100644
 --- a/src/udev/udevd.c
 +++ b/src/udev/udevd.c
 @@ -1197,7 +1197,8 @@ int main(int argc, char *argv[])
  }
  log_debug(set children_max to %u\n, children_max);

 -udev_rules_apply_static_dev_perms(rules);
 +if (udev_rules_apply_static_dev_perms(rules)  0)
 +log_error(failed to apply permissions on static device 
 nodes\n);

 Hmm, not going to use the return from udev_rules_apply_static_dev_perms
 to add to the error?

Fixed.

Thanks,

Tom
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCHv2 1/2] udev: export tags of dead device nodes to /run/udev/static_node-tags/

2013-07-14 Thread Dave Reisner 
On Sun, Jul 14, 2013 at 02:35:51PM +0200, Tom Gundersen wrote:
 Based on a patch by Kay Sievers.
 
 A tag is exported at boot as a symlinks to the device node in the folder
 /run/udev/static_node-tags/tagname/, if the device node exists.
 
 These tags are cleaned up by udevadm info --cleanup-db, but are otherwise
 never removed.
 ---
 
 v2: use directories of symlinks, rather than a flat file
 
  man/udev.xml   | 10 --
  src/login/70-uaccess.rules |  3 +-
  src/udev/udev-rules.c  | 86 
 +++---
  src/udev/udev.h|  2 +-
  src/udev/udevadm-info.c|  6 
  src/udev/udevd.c   |  3 +-
  6 files changed, 91 insertions(+), 19 deletions(-)
 
 diff --git a/man/udev.xml b/man/udev.xml
 index 553bbfd..ca8444c 100644
 --- a/man/udev.xml
 +++ b/man/udev.xml
 @@ -521,9 +521,13 @@
  termoptionstatic_node=/option/term
  listitem
paraApply the permissions specified in this rule to the 
 static device node with
 -  the specified name. Static device node creation can be 
 requested by kernel modules.
 -  These nodes might not have a corresponding kernel device 
 at the time systemd-udevd is
 -  started; they can trigger automatic kernel module 
 loading./para
 +  the specified name. Also, for every tag specified in this 
 rule, create a symlink
 +  in the directory
 +  
 filename/run/udev/static_node-tags/replaceabletag/replaceable/filename
 +  pointing at the static device node with the specified 
 name. Static device node
 +  creation is performed by systemd-tmpfiles before 
 systemd-udevd is started. The
 +  static nodes might not have a corresponding kernel device; 
 they are used to
 +  trigger automatic kernel module loading when they are 
 accessed./para
  /listitem
/varlistentry
varlistentry
 diff --git a/src/login/70-uaccess.rules b/src/login/70-uaccess.rules
 index a118f8e..01484c9 100644
 --- a/src/login/70-uaccess.rules
 +++ b/src/login/70-uaccess.rules
 @@ -25,7 +25,8 @@ SUBSYSTEM==block, ENV{ID_CDROM}==1, TAG+=uaccess
  SUBSYSTEM==scsi_generic, SUBSYSTEMS==scsi, ATTRS{type}==4|5, 
 TAG+=uaccess
  
  # Sound devices
 -SUBSYSTEM==sound, TAG+=uaccess
 +SUBSYSTEM==sound, TAG+=uaccess \
 +  OPTIONS+=static_node=snd/timer, OPTIONS+=static_node=snd/seq
  
  # ffado is an userspace driver for firewire sound cards
  SUBSYSTEM==firewire, ENV{ID_FFADO}==1, TAG+=uaccess
 diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c
 index fe65e2d..9d74e3a 100644
 --- a/src/udev/udev-rules.c
 +++ b/src/udev/udev-rules.c
 @@ -33,6 +33,7 @@
  #include path-util.h
  #include conf-files.h
  #include strbuf.h
 +#include strv.h
  
  #define PREALLOC_TOKEN  2048
  
 @@ -152,9 +153,9 @@ enum token_type {
  TK_A_OWNER_ID,  /* uid_t */
  TK_A_GROUP_ID,  /* gid_t */
  TK_A_MODE_ID,   /* mode_t */
 +TK_A_TAG,   /* val */
  TK_A_STATIC_NODE,   /* val */
  TK_A_ENV,   /* val, attr */
 -TK_A_TAG,   /* val */

Is this swap really needed? Seems like noise...

  TK_A_NAME,  /* val */
  TK_A_DEVLINK,   /* val */
  TK_A_ATTR,  /* val, attr */
 @@ -2496,16 +2497,21 @@ int udev_rules_apply_to_event(struct udev_rules 
 *rules, struct udev_event *event
  }
  }
  
 -void udev_rules_apply_static_dev_perms(struct udev_rules *rules)
 +int udev_rules_apply_static_dev_perms(struct udev_rules *rules)
  {
  struct token *cur;
  struct token *rule;
  uid_t uid = 0;
  gid_t gid = 0;
  mode_t mode = 0;
 +_cleanup_strv_free_ char **tags = NULL;
 +char **t;
 +FILE *f = NULL;
 +_cleanup_free_ char *path = NULL;
 +int r = 0;
  
  if (rules-tokens == NULL)
 -return;
 +return 0;
  
  cur = rules-tokens[0];
  rule = cur;
 @@ -2522,6 +2528,8 @@ void udev_rules_apply_static_dev_perms(struct 
 udev_rules *rules)
  uid = 0;
  gid = 0;
  mode = 0;
 +strv_free(tags);
 +tags = NULL;
  break;
  case TK_A_OWNER_ID:
  uid = cur-key.uid;
 @@ -2532,18 +2540,56 @@ void udev_rules_apply_static_dev_perms(struct 
 udev_rules *rules)
  case TK_A_MODE_ID:
  mode = cur-key.mode;
  break;
 +case TK_A_TAG:
 +t = strv_append(tags, rules_str(rules,