[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] Rule Names



Hello,

I've made some changes to this patchset. I mostly cleaned it up
(getting rid of unnessecary explicit casts, mostly), but made
a few additional changes:

On Tue, Dec 25, 2012 at 12:13:33AM -0800, Tyler Thomas Hart wrote:
>     a name that represents the index of the rule. But this is still the string. It is sprinted
>     in (don't worry, it should be safe the way I did it). The index itself is a global unsigned

I've changed this to using snprintf(). Buffer overflows were not
an issue in either case, but it should provide blanket security
if more changes are made in the future.

> 'unrule' searches using strcmp incrementing through the rule queue. It seems like this would be
>     slow (its not really noticible) but I didn't know of a better way.

I suppose I should clarify this statement. What I mean is: it would
be better to use a function that exits on the first mismatched char,
since we only care about exact matches. (Looking at the string.c, it
doesnt look like strcmp() does this). Afaik, a function like this
doesn't exist in stdlib/glib/xlib, so I've decided to stick to strcmp.

If anyone knows where else in hlwm source a function like this could
be used, or anyone knows of any lib funtions that does this, we can
change this in a later commit (it would be easy to write a function
that does this, but the efficiency it would provide to just ONE
function is not worth the extra lines of code, IMO).

> Add command 'list_rules', including rule names.

I changed the output format of list_rules() to be more like the
output of list_monitors, eg '$RULENAME: foo bar', and 'foo bar'
are just the arguments the rule command was called with. Each arg
is tab-seperated, since args can contain spaces.

I've also rebased and repatched, since the NEWS file changed to 5.1,
along with other changes to src, the old patchset was having a lot of
rejected hunks.


Tylo
>From 2e5adf91742e384c39583e2e9405d7c997d9cbf3 Mon Sep 17 00:00:00 2001
From: Tyler Thomas Hart <htyler _at_ pdx _dot_ edu>
Date: Sat, 15 Dec 2012 15:11:50 -0800
Subject: [PATCH 1/3] Adds initial support for rule names.

A new char* member in the HSRule struct is added for names. New
property for rule command 'name=' added, to add custom names. New
rules without an explicit name is given an integer name. Adds flag
printname which prints the name of a newly created rule to stdout.
---
 NEWS                 |  4 +++-
 doc/herbstluftwm.txt | 19 ++++++++++++++-----
 src/rules.c          | 26 ++++++++++++++++++++++++++
 src/rules.h          |  1 +
 4 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/NEWS b/NEWS
index 3bfc9b6..e6c8e4e 100644
--- a/NEWS
+++ b/NEWS
@@ -9,7 +9,9 @@ Changes:
     * disallow focus_follows_mouse if the focus change hides another window
       (e.g. an pseudotiled window in the max layout). In that case an extra
       click is required to change the focus.
-
+    * rule names: rules can be given names with the 'name' property. The name
+      can be printed to stdout using the 'printname' flag to the rule command.
+ 
 Release 0.5.1 on 2013-01-05
 ---------------------------
 
diff --git a/doc/herbstluftwm.txt b/doc/herbstluftwm.txt
index 4345dda..687a175 100644
--- a/doc/herbstluftwm.txt
+++ b/doc/herbstluftwm.txt
@@ -649,7 +649,7 @@ floating [['TAG'] *on*|*off*|*toggle*|*status*]::
     is given, floating mode is toggled. If status is given, then *on* or *off*
     is printed, depending of the floating state on 'TAG'.
 
-rule [\[--]'FLAG'|\[--]'CONDITION'|\[--]'CONSEQUENCE' ...]::
+rule [\[--]'FLAG'|\[--]'NAME'|\[--]'CONDITION'|\[--]'CONSEQUENCE' ...]::
     Defines a rule which will be applied to all new clients. Its behaviour is
     described in the <<RULES,*RULES section*>>.
 
@@ -870,11 +870,19 @@ appear. Each rule matches against a certain subset of all clients and defines a
 set of properties for them (called 'CONSEQUENCE'##s##). A rule can be defined
 with this command:
 
-+rule+ [\[--]'FLAG'|\[--]'CONDITION'|\[--]'CONSEQUENCE' ...]
++rule+ [\[--]'FLAG'|\[--]'NAME'|\[--]'CONDITION'|\[--]'CONSEQUENCE' ...]
 
-Each rule consists of a list of 'FLAG'##s##, 'CONDITION'##s## and
-'CONSEQUENCE'##s## (each of them can be optionally prefixed with two dashes
-(+--+) to provide a more *iptables*(8)-like feeling).
+Each rule consists of a list of 'FLAG'##s##, 'CONDITION'##s## 'CONSEQUENCE'##s##
+and, optionally, a 'NAME'. (each of them can be optionally prefixed with two
+dashes (+--+) to provide a more *iptables*(8)-like feeling).
+
+Each rule can be given a custom name by specifying the 'NAME' property:
+
+    * +NAME+='VALUE'
+
+If multiple name properties are specified, the last one in the list will be
+applied. If no name is given, then the rule will be given a name that is an
+integer, representing the index of the rule since the first rule was applied.
 
 If a new client
 appears, herbstluftwm tries to apply each rule to this new client as follows:
@@ -969,6 +977,7 @@ A rule's behaviour can be configured by some special 'FLAGS':
     * +not+: negates the next 'CONDITION'.
     * +!+: same as +not+.
     * +once+: only apply this rule once (and delete it afterwards).
+    * +printname+: prints the name of the newly created rule to stdout.
 
 Examples:
 
diff --git a/src/rules.c b/src/rules.c
index eb2891b..acaef55 100644
--- a/src/rules.c
+++ b/src/rules.c
@@ -77,6 +77,7 @@ static HSConditionType g_condition_types[] = {
 
 int     g_maxage_type; // index of "maxage"
 time_t  g_current_rule_birth_time; // data from rules_apply() to condition_maxage()
+unsigned long long g_rule_id_index; // incremental index of rule ID
 
 static HSConsequenceType g_consequence_types[] = {
     { "tag",            consequence_tag             },
@@ -97,11 +98,13 @@ GQueue g_rules = G_QUEUE_INIT; // a list of HSRule* elements
 // RULES //
 void rules_init() {
     g_maxage_type = find_condition_type("maxage");
+    g_rule_id_index = 0;
 }
 
 void rules_destroy() {
     g_queue_foreach(&g_rules, (GFunc)rule_destroy, NULL);
     g_queue_clear(&g_rules);
+    g_rule_id_index = 0;
 }
 
 // condition types //
@@ -236,6 +239,10 @@ HSRule* rule_create() {
     HSRule* rule = g_new0(HSRule, 1);
     rule->once = false;
     rule->birth_time = get_monotonic_timestamp();
+    // Add name. Defaults to index number.
+    char name[22]; // 20 is the max decimal digits a u-l-l can be.
+    snprintf(name, sizeof(char) * 22, "%llu", g_rule_id_index++);
+    rule->name = g_strdup(name);
     return rule;
 }
 
@@ -250,6 +257,8 @@ void rule_destroy(HSRule* rule) {
         consequence_destroy(rule->consequences[i]);
     }
     g_free(rule->consequences);
+    // free name
+    g_free(rule->name);
     // free rule itself
     g_free(rule);
 }
@@ -304,6 +313,7 @@ int rule_add_command(int argc, char** argv, GString* output) {
     HSRule* rule = rule_create();
     HSCondition* cond;
     HSConsequence* cons;
+    bool printname = false;
     bool negated = false;
     struct {
         char* name;
@@ -312,6 +322,7 @@ int rule_add_command(int argc, char** argv, GString* output) {
         { "not",    &negated },
         { "!",      &negated },
         { "once",   &rule->once },
+        { "printname",&printname },
     };
 
     // parse rule incrementally. always maintain a correct rule in rule
@@ -358,6 +369,17 @@ int rule_add_command(int argc, char** argv, GString* output) {
             rule_add_consequence(rule, cons);
         }
 
+        // Check for a provided name, and replace index if so
+        else if (consorcond && (!strcmp(name,"name"))) {
+            if (value[0] == '-') {
+                g_string_append_printf(output, "rule: name cannot begin with '-' (dash)");
+                return HERBST_INVALID_ARGUMENT;
+            } else {
+                g_free(rule->name);
+                rule->name = g_strdup(value);
+            }
+        }
+
         else {
             // need to hardcode "rule:" here because args are shifted
             g_string_append_printf(output,
@@ -367,6 +389,10 @@ int rule_add_command(int argc, char** argv, GString* output) {
         }
     }
 
+    if (printname) {
+       g_string_append_printf(output, "%s", rule->name);
+    }
+
     g_queue_push_tail(&g_rules, rule);
     return 0;
 }
diff --git a/src/rules.h b/src/rules.h
index 9384711..693a372 100644
--- a/src/rules.h
+++ b/src/rules.h
@@ -43,6 +43,7 @@ typedef struct {
 } HSConsequence;
 
 typedef struct {
+    char*           name;
     HSCondition**   conditions;
     int             condition_count;
     HSConsequence** consequences;
-- 
1.8.0.3

>From b4526bb0a29a4552edbc96a33a8c7b1a5f314276 Mon Sep 17 00:00:00 2001
From: Tyler Thomas Hart <htyler _at_ pdx _dot_ edu>
Date: Sat, 15 Dec 2012 15:49:14 -0800
Subject: [PATCH 2/3] Allows unrule command to remove by name

---
 NEWS                 |  3 ++-
 doc/herbstluftwm.txt |  6 +++---
 src/rules.c          | 35 ++++++++++++++++++++++++++++++++---
 3 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/NEWS b/NEWS
index e6c8e4e..0bf7517 100644
--- a/NEWS
+++ b/NEWS
@@ -11,7 +11,8 @@ Changes:
       click is required to change the focus.
     * rule names: rules can be given names with the 'name' property. The name
       can be printed to stdout using the 'printname' flag to the rule command.
- 
+      Unrule command accepts names, and can remove rules individually.
+
 Release 0.5.1 on 2013-01-05
 ---------------------------
 
diff --git a/doc/herbstluftwm.txt b/doc/herbstluftwm.txt
index 687a175..524e5b4 100644
--- a/doc/herbstluftwm.txt
+++ b/doc/herbstluftwm.txt
@@ -653,9 +653,9 @@ rule [\[--]'FLAG'|\[--]'NAME'|\[--]'CONDITION'|\[--]'CONSEQUENCE' ...]::
     Defines a rule which will be applied to all new clients. Its behaviour is
     described in the <<RULES,*RULES section*>>.
 
-unrule *--all*|*-F*::
-    If --all or -F is passed, then all rules are removed.
-    (It is not possible yet to remove certain rules)
+unrule 'NAME'|*--all*|*-F*::
+    Removes all rules named 'NAME'. If --all or -F is passed, then all rules are
+    removed.
 
 fullscreen [*on*|*off*|*toggle*]::
     Sets or toggles the fullscreen state of the focused client. If no argument
diff --git a/src/rules.c b/src/rules.c
index acaef55..0f59734 100644
--- a/src/rules.c
+++ b/src/rules.c
@@ -263,6 +263,31 @@ void rule_destroy(HSRule* rule) {
     g_free(rule);
 }
 
+// Compares the name of two rules.
+static gint rule_compare_name(const HSRule* a, const HSRule* b) {
+    return (!strcmp(a->name, b->name)) ? 0 : -1;
+}
+
+// Looks up a rule with a given index and removes if from the queue
+bool rule_find_pop(char* name) {
+    GList* rule = { NULL };
+    bool status = false; // Will be returned as true if any is found
+    HSRule rule_find = { .name = name };
+    while ((rule = g_queue_find_custom(&g_rules, &rule_find,
+                        (GCompareFunc)rule_compare_name))) {
+        // Check if rule with name exists
+        if ( rule == NULL ) {
+            break;
+        }
+        status = true;
+        // If so, clear data
+        rule_destroy(rule->data);
+        // Remove and free empty link
+        g_queue_delete_link(&g_rules, rule);
+    }
+    return status;
+}
+
 // parses an arg like NAME=VALUE to res_name, res_operation and res_value
 bool tokenize_arg(char* condition,
                   char** res_name, char* res_operation, char** res_value) {
@@ -406,12 +431,16 @@ int rule_remove_command(int argc, char** argv, GString* output) {
         // remove all rules
         g_queue_foreach(&g_rules, (GFunc)rule_destroy, NULL);
         g_queue_clear(&g_rules);
+        g_rule_id_index = 0;
         return 0;
     }
 
-    g_string_append_printf(output,
-        "%s: This command must be used with --all/-F\n", argv[0]);
-    return HERBST_INVALID_ARGUMENT;
+    // Deletes rule with given name
+    if (!rule_find_pop(argv[1])) {
+        g_string_append_printf(output, "Couldn't find rule: %s", argv[1]);
+        return HERBST_INVALID_ARGUMENT;
+    }
+    return 0;
 }
 
 // rules applying //
-- 
1.8.0.3

>From a4bf6aa7ec4bcb04d3660ed822c027f15d5d99f0 Mon Sep 17 00:00:00 2001
From: Tyler Thomas Hart <htyler _at_ pdx _dot_ edu>
Date: Sat, 15 Dec 2012 16:16:28 -0800
Subject: [PATCH 3/3] Add command list_rules

---
 NEWS                 |  1 +
 doc/herbstluftwm.txt |  3 +++
 src/command.c        |  1 +
 src/main.c           |  1 +
 src/rules.c          | 56 ++++++++++++++++++++++++++++++++++++++++++++++++----
 src/rules.h          |  6 +++++-
 6 files changed, 63 insertions(+), 5 deletions(-)

diff --git a/NEWS b/NEWS
index 0bf7517..f42b8c3 100644
--- a/NEWS
+++ b/NEWS
@@ -12,6 +12,7 @@ Changes:
     * rule names: rules can be given names with the 'name' property. The name
       can be printed to stdout using the 'printname' flag to the rule command.
       Unrule command accepts names, and can remove rules individually.
+    * new command: list_rules
 
 Release 0.5.1 on 2013-01-05
 ---------------------------
diff --git a/doc/herbstluftwm.txt b/doc/herbstluftwm.txt
index 524e5b4..b5a7274 100644
--- a/doc/herbstluftwm.txt
+++ b/doc/herbstluftwm.txt
@@ -162,6 +162,9 @@ list_commands::
     List currently configured monitors with their index, area (as rectangle),
     name (if named) and currently viewed tag.
 
+list_rules::
+    Lists all active rules with their associated names.
+
 list_keybinds::
     Lists all bound keys with their associated command. Each line consists of
     one key combination and the command with its parameters separated by tabs.
diff --git a/src/command.c b/src/command.c
index 62c54d3..49cff6b 100644
--- a/src/command.c
+++ b/src/command.c
@@ -58,6 +58,7 @@ struct {
     { "list_commands",  1,  no_completion },
     { "list_monitors",  1,  no_completion },
     { "list_keybinds",  1,  no_completion },
+    { "list_rules",     1,  no_completion },
     { "lock",           1,  no_completion },
     { "unlock",         1,  no_completion },
     { "keybind",        2,  keybind_parameter_expected },
diff --git a/src/main.c b/src/main.c
index a1dc7a8..6ad0460 100644
--- a/src/main.c
+++ b/src/main.c
@@ -145,6 +145,7 @@ CommandBinding g_commands[] = {
     CMD_BIND(             "raise",          raise_command),
     CMD_BIND(             "rule",           rule_add_command),
     CMD_BIND(             "unrule",         rule_remove_command),
+    CMD_BIND(             "list_rules",     rule_print_all_command),
     CMD_BIND(             "layout",         print_layout_command),
     CMD_BIND(             "stack",          print_stack_command),
     CMD_BIND(             "dump",           print_layout_command),
diff --git a/src/rules.c b/src/rules.c
index 0f59734..c9df7f9 100644
--- a/src/rules.c
+++ b/src/rules.c
@@ -144,15 +144,16 @@ HSCondition* condition_create(int type, char op, char* value, GString* output) {
 
         case '~':
             cond.value_type = CONDITION_VALUE_TYPE_REGEX;
-            int status = regcomp(&cond.value.exp, value, REG_EXTENDED);
+            int status = regcomp(&cond.value.reg.exp, value, REG_EXTENDED);
             if (status != 0) {
                 char buf[ERROR_STRING_BUF_SIZE];
-                regerror(status, &cond.value.exp, buf, ERROR_STRING_BUF_SIZE);
+                regerror(status, &cond.value.reg.exp, buf, ERROR_STRING_BUF_SIZE);
                 g_string_append_printf(output,
                     "rule: Can not parse value \"%s\" from condition \"%s\": \"%s\"\n",
                     value, g_condition_types[type].name, buf);
                 return NULL;
             }
+            cond.value.reg.str = g_strdup(value);
             break;
 
         default:
@@ -179,7 +180,8 @@ void condition_destroy(HSCondition* cond) {
             free(cond->value.str);
             break;
         case CONDITION_VALUE_TYPE_REGEX:
-            regfree(&cond->value.exp);
+            regfree(&cond->value.reg.exp);
+            g_free(cond->value.reg.str);
             break;
         default:
             break;
@@ -288,6 +290,52 @@ bool rule_find_pop(char* name) {
     return status;
 }
 
+// List all rules in queue
+static void rule_print_append_output(HSRule* rule, GString* output) {
+    g_string_append_printf(output, "%s:", rule->name);
+#define RULE_PRINT_TAB g_string_append_c(output, '\t');
+    RULE_PRINT_TAB;
+    // Append conditions
+    for (int i = 0; i < rule->condition_count; i++) {
+        g_string_append_printf(output, "%s=",
+            g_condition_types[rule->conditions[i]->condition_type].name);
+        switch (rule->conditions[i]->value_type) {
+            case CONDITION_VALUE_TYPE_STRING:
+                g_string_append_printf(output, "%s",
+                    rule->conditions[i]->value.str);
+                RULE_PRINT_TAB;
+                break;
+            case CONDITION_VALUE_TYPE_REGEX:
+                g_string_append_printf(output, "%s",
+                    rule->conditions[i]->value.reg.str);
+                RULE_PRINT_TAB;
+                break;
+            default: /* CONDITION_VALUE_TYPE_INTEGER: */
+                g_string_append_printf(output, "%i",
+                    rule->conditions[i]->value.integer);
+                RULE_PRINT_TAB;
+                break;
+        }
+    }
+    // Append consequences
+    for (int i = 0; i < rule->consequence_count; i++) {
+        g_string_append_printf(output, "%s=",
+            g_consequence_types[rule->consequences[i]->type].name);
+        g_string_append_printf(output, "%s",
+            rule->consequences[i]->value.str);
+        RULE_PRINT_TAB;
+    }
+#undef RULE_PRINT_TAB
+    // Print new line
+    g_string_append_c(output, '\n');
+}
+
+int rule_print_all_command(int argc, char** argv, GString* output) {
+    // Print entry for each in the queue
+    g_queue_foreach(&g_rules, (GFunc)rule_print_append_output, output);
+    return 0;
+}
+
 // parses an arg like NAME=VALUE to res_name, res_operation and res_value
 bool tokenize_arg(char* condition,
                   char** res_name, char* res_operation, char** res_value) {
@@ -537,7 +585,7 @@ bool condition_string(HSCondition* rule, char* string) {
             return !strcmp(string, rule->value.str);
             break;
         case CONDITION_VALUE_TYPE_REGEX:
-            status = regexec(&rule->value.exp, string, 1, &match, 0);
+            status = regexec(&rule->value.reg.exp, string, 1, &match, 0);
             // only accept it, if it matches the entire string
             if (status == 0
                 && match.rm_so == 0
diff --git a/src/rules.h b/src/rules.h
index 693a372..d6ba45a 100644
--- a/src/rules.h
+++ b/src/rules.h
@@ -29,7 +29,10 @@ typedef struct {
     bool negated;
     union {
         char*       str;
-        regex_t     exp;
+        struct {
+            regex_t     exp;
+            char*       str;
+        } reg;
         int         integer;
     } value;
 } HSCondition;
@@ -74,6 +77,7 @@ void rule_destroy(HSRule* rule);
 
 int rule_add_command(int argc, char** argv, GString* output);
 int rule_remove_command(int argc, char** argv, GString* output);
+int rule_print_all_command(int argc, char** argv, GString* output);
 
 #endif
 
-- 
1.8.0.3