map_cast_from_hex() does not produce error messages
[freeradius.git] / src / main / modcall.c
index 6fae3cb..b3dd3f0 100644 (file)
@@ -73,7 +73,7 @@ typedef struct {
        modcallable             *children;
        modcallable             *tail;          /* of the children list */
        CONF_SECTION            *cs;
-       value_pair_map_t        *map;           /* update */
+       vp_map_t        *map;           /* update */
        vp_tmpl_t       *vpt;           /* switch */
        fr_cond_t               *cond;          /* if/elsif */
        bool                    done_pass2;
@@ -283,9 +283,9 @@ static rlm_rcode_t CC_HINT(nonnull) call_modsingle(rlm_components_t component, m
        blocked = (request->master_state == REQUEST_STOP_PROCESSING);
        if (blocked) return RLM_MODULE_NOOP;
 
-       RDEBUG3("modsingle[%s]: calling %s (%s) for request %d",
+       RDEBUG3("modsingle[%s]: calling %s (%s)",
                comp2str[component], sp->modinst->name,
-               sp->modinst->entry->name, request->number);
+               sp->modinst->entry->name);
        request->log.indent = 0;
 
        if (sp->modinst->force) {
@@ -309,19 +309,19 @@ static rlm_rcode_t CC_HINT(nonnull) call_modsingle(rlm_components_t component, m
         */
        blocked = (request->master_state == REQUEST_STOP_PROCESSING);
        if (blocked) {
-               RWARN("Module %s became unblocked for request %u", sp->modinst->entry->name, request->number);
+               RWARN("Module %s became unblocked", sp->modinst->entry->name);
        }
 
  fail:
        request->log.indent = indent;
-       RDEBUG3("modsingle[%s]: returned from %s (%s) for request %d",
+       RDEBUG3("modsingle[%s]: returned from %s (%s)",
               comp2str[component], sp->modinst->name,
-              sp->modinst->entry->name, request->number);
+              sp->modinst->entry->name);
 
        return request->rcode;
 }
 
-static int default_component_results[RLM_COMPONENT_COUNT] = {
+static int default_component_results[MOD_COUNT] = {
        RLM_MODULE_REJECT,      /* AUTH */
        RLM_MODULE_NOTFOUND,    /* AUTZ */
        RLM_MODULE_NOOP,        /* PREACCT */
@@ -380,14 +380,14 @@ typedef struct modcall_stack_entry_t {
 
 
 static bool modcall_recurse(REQUEST *request, rlm_components_t component, int depth,
-                           modcall_stack_entry_t *entry);
+                           modcall_stack_entry_t *entry, bool do_next_sibling);
 
 /*
  *     Call a child of a block.
  */
 static void modcall_child(REQUEST *request, rlm_components_t component, int depth,
                          modcall_stack_entry_t *entry, modcallable *c,
-                         rlm_rcode_t *result)
+                         rlm_rcode_t *result, bool do_next_sibling)
 {
        modcall_stack_entry_t *next;
 
@@ -406,7 +406,7 @@ static void modcall_child(REQUEST *request, rlm_components_t component, int dept
        next->unwind = 0;
 
        if (!modcall_recurse(request, component,
-                            depth, next)) {
+                            depth, next, do_next_sibling)) {
                *result = RLM_MODULE_FAIL;
                 return;
        }
@@ -428,7 +428,7 @@ static void modcall_child(REQUEST *request, rlm_components_t component, int dept
  *     Interpret the various types of blocks.
  */
 static bool modcall_recurse(REQUEST *request, rlm_components_t component, int depth,
-                           modcall_stack_entry_t *entry)
+                           modcall_stack_entry_t *entry, bool do_next_sibling)
 {
        bool if_taken, was_if;
        modcallable *c;
@@ -449,6 +449,10 @@ redo:
         */
        if (!c) goto finish;
 
+       if (fr_debug_lvl >= 3) {
+               VERIFY_REQUEST(request);
+       }
+
        rad_assert(c->debug_name != NULL); /* if this happens, all bets are off. */
 
        /*
@@ -514,8 +518,8 @@ redo:
                 *      Like MOD_ELSE, but allow for a later "else"
                 */
                if (if_taken) {
-                       RDEBUG2("... skipping %s for request %d: Preceding \"if\" was taken",
-                               unlang_keyword[c->type], request->number);
+                       RDEBUG2("... skipping %s: Preceding \"if\" was taken",
+                               unlang_keyword[c->type]);
                        was_if = true;
                        if_taken = true;
                        goto next_sibling;
@@ -533,14 +537,14 @@ redo:
        if (c->type == MOD_ELSE) {
                if (!was_if) { /* error */
                elsif_error:
-                       RDEBUG2("... skipping %s for request %d: No preceding \"if\"",
-                               unlang_keyword[c->type], request->number);
+                       RDEBUG2("... skipping %s: No preceding \"if\"",
+                               unlang_keyword[c->type]);
                        goto next_sibling;
                }
 
                if (if_taken) {
-                       RDEBUG2("... skipping %s for request %d: Preceding \"if\" was taken",
-                               unlang_keyword[c->type], request->number);
+                       RDEBUG2("... skipping %s: Preceding \"if\" was taken",
+                               unlang_keyword[c->type]);
                        was_if = false;
                        if_taken = false;
                        goto next_sibling;
@@ -584,7 +588,7 @@ redo:
        if (c->type == MOD_UPDATE) {
                int rcode;
                modgroup *g = mod_callabletogroup(c);
-               value_pair_map_t *map;
+               vp_map_t *map;
 
                MOD_LOG_OPEN_BRACE;
                RINDENT();
@@ -659,7 +663,7 @@ redo:
                     vp != NULL;
                     vp = fr_cursor_next(&copy)) {
 #ifndef NDEBUG
-                       if (fr_debug_flag >= 2) {
+                       if (fr_debug_lvl >= 2) {
                                char buffer[1024];
 
                                vp_prints_value(buffer, sizeof(buffer), vp, '"');
@@ -682,15 +686,17 @@ redo:
                        next->priority = 0;
                        next->unwind = 0;
 
-                       if (!modcall_recurse(request, component, depth + 1, next)) {
+                       if (!modcall_recurse(request, component, depth + 1, next, true)) {
                                break;
                        }
 
                        /*
-                        *      We've unwound to the enclosing
-                        *      "foreach".  Stop the unwinding.
+                        *      We've been asked to unwind to the
+                        *      enclosing "foreach".  We're here, so
+                        *      we can stop unwinding.
                         */
-                       if (next->unwind == MOD_FOREACH) {
+                       if (next->unwind == MOD_BREAK) {
+                               entry->unwind = 0;
                                break;
                        }
 
@@ -708,7 +714,7 @@ redo:
                 *      If we don't remove the request data, something could call
                 *      the xlat outside of a foreach loop and trigger a segv.
                 */
-               pairfree(&vps);
+               fr_pair_list_free(&vps);
                request_data_get(request, (void *)radius_get_vp, foreach_depth);
 
                rad_assert(next != NULL);
@@ -770,6 +776,11 @@ redo:
                 *      MOD_GROUP.
                 */
                if (!g->children) {
+                       if (c->type == MOD_CASE) {
+                               result = RLM_MODULE_NOOP;
+                               goto calculate_result;
+                       }
+
                        RDEBUG2("%s { ... } # empty sub-section is ignored", c->name);
                        goto next_sibling;
                }
@@ -777,7 +788,7 @@ redo:
                MOD_LOG_OPEN_BRACE;
                modcall_child(request, component,
                              depth + 1, entry, g->children,
-                             &result);
+                             &result, true);
                MOD_LOG_CLOSE_BRACE;
                goto calculate_result;
        } /* MOD_GROUP */
@@ -788,7 +799,7 @@ redo:
                modgroup *g, *h;
                fr_cond_t cond;
                value_data_t data;
-               value_pair_map_t map;
+               vp_map_t map;
                vp_tmpl_t vpt;
 
                MOD_LOG_OPEN_BRACE;
@@ -912,7 +923,7 @@ redo:
 
        do_null_case:
                talloc_free(data.ptr);
-               modcall_child(request, component, depth + 1, entry, found, &result);
+               modcall_child(request, component, depth + 1, entry, found, &result, true);
                MOD_LOG_CLOSE_BRACE;
                goto calculate_result;
        } /* MOD_SWITCH */
@@ -941,12 +952,10 @@ redo:
                        }
                }
 
-               MOD_LOG_OPEN_BRACE;
-
                if (c->type == MOD_LOAD_BALANCE) {
                        modcall_child(request, component,
                                      depth + 1, entry, found,
-                                     &result);
+                                     &result, false);
 
                } else {
                        this = found;
@@ -954,7 +963,7 @@ redo:
                        do {
                                modcall_child(request, component,
                                              depth + 1, entry, this,
-                                             &result);
+                                             &result, false);
                                if (this->actions[result] == MOD_ACTION_RETURN) {
                                        priority = -1;
                                        break;
@@ -1005,7 +1014,7 @@ redo:
                        radius_xlat(buffer, sizeof(buffer), request, mx->xlat_name, NULL, NULL);
                } else {
                        RDEBUG("`%s`", mx->xlat_name);
-                       radius_exec_program(NULL, 0, NULL, request, mx->xlat_name, request->packet->vps,
+                       radius_exec_program(request, NULL, 0, NULL, request, mx->xlat_name, request->packet->vps,
                                            false, true, EXEC_TIMEOUT);
                }
 
@@ -1077,7 +1086,6 @@ calculate_result:
         */
        if (entry->unwind == MOD_BREAK) {
                RDEBUG2("# unwind to enclosing foreach");
-               entry->unwind = 0;
                goto finish;
        }
 
@@ -1086,9 +1094,11 @@ calculate_result:
        }
 
 next_sibling:
-       entry->c = entry->c->next;
+       if (do_next_sibling) {
+               entry->c = entry->c->next;
 
-       if (entry->c) goto redo;
+               if (entry->c) goto redo;
+       }
 
 finish:
        /*
@@ -1121,7 +1131,7 @@ int modcall(rlm_components_t component, modcallable *c, REQUEST *request)
        /*
         *      Call the main handler.
         */
-       if (!modcall_recurse(request, component, 0, &stack[0])) {
+       if (!modcall_recurse(request, component, 0, &stack[0], true)) {
                return RLM_MODULE_FAIL;
        }
 
@@ -1186,7 +1196,7 @@ static void dump_tree(rlm_components_t comp, modcallable *c)
  * behaves like the code from the old module_*() function. redundant{}
  * are based on my guesses of what they will be used for. --Pac. */
 static const int
-defaultactions[RLM_COMPONENT_COUNT][GROUPTYPE_COUNT][RLM_MODULE_NUMCODES] =
+defaultactions[MOD_COUNT][GROUPTYPE_COUNT][RLM_MODULE_NUMCODES] =
 {
        /* authenticate */
        {
@@ -1497,7 +1507,7 @@ static const int authtype_actions[GROUPTYPE_COUNT][RLM_MODULE_NUMCODES] =
  * @param ctx data to pass to fixup function (currently unused).
  * @return 0 if valid else -1.
  */
-int modcall_fixup_update(value_pair_map_t *map, UNUSED void *ctx)
+int modcall_fixup_update(vp_map_t *map, UNUSED void *ctx)
 {
        CONF_PAIR *cp = cf_item_to_pair(map->ci);
 
@@ -1550,22 +1560,11 @@ int modcall_fixup_update(value_pair_map_t *map, UNUSED void *ctx)
        /*
         *      Depending on the attribute type, some operators are disallowed.
         */
-       if (map->lhs->type == TMPL_TYPE_ATTR) {
-               switch (map->op) {
-               default:
-                       cf_log_err(map->ci, "Invalid operator for attribute");
-                       return -1;
-
-               case T_OP_EQ:
-               case T_OP_CMP_EQ:
-               case T_OP_ADD:
-               case T_OP_SUB:
-               case T_OP_LE:
-               case T_OP_GE:
-               case T_OP_CMP_FALSE:
-               case T_OP_SET:
-                       break;
-               }
+       if ((map->lhs->type == TMPL_TYPE_ATTR) && (!fr_assignment_op[map->op] && !fr_equality_op[map->op])) {
+               cf_log_err(map->ci, "Invalid operator \"%s\" in update section.  "
+                          "Only assignment or filter operators are allowed",
+                          fr_int2str(fr_tokens, map->op, "<INVALID>"));
+               return -1;
        }
 
        if (map->lhs->type == TMPL_TYPE_LIST) {
@@ -1605,7 +1604,7 @@ int modcall_fixup_update(value_pair_map_t *map, UNUSED void *ctx)
 
                case T_OP_SET:
                        if (map->rhs->type == TMPL_TYPE_EXEC) {
-                               WARN("%s[%d] Please change ':=' to '=' for list assignment",
+                               WARN("%s[%d]: Please change ':=' to '=' for list assignment",
                                     cf_pair_filename(cp), cf_pair_lineno(cp));
                        }
 
@@ -1651,8 +1650,26 @@ int modcall_fixup_update(value_pair_map_t *map, UNUSED void *ctx)
                    (map->lhs->tmpl_da->type == PW_TYPE_STRING) &&
                    (cf_pair_value_type(cp) == T_SINGLE_QUOTED_STRING)) {
                        tmpl_cast_in_place_str(map->rhs);
+
                } else {
-                       if (tmpl_cast_in_place(map->rhs, map->lhs->tmpl_da->type, map->lhs->tmpl_da) < 0) {
+                       /*
+                        *      RHS is hex, try to parse it as
+                        *      type-specific data.
+                        */
+                       if (map->lhs->auto_converted &&
+                           (map->rhs->name[0] == '0') && (map->rhs->name[1] == 'x') &&
+                           (map->rhs->len > 2) && ((map->rhs->len & 0x01) == 0)) {
+                               vp_tmpl_t *vpt = map->rhs;
+                               map->rhs = NULL;
+
+                               if (!map_cast_from_hex(map, T_BARE_WORD, vpt->name)) {
+                                       map->rhs = vpt;
+                                       cf_log_err(map->ci, "Cannot parse RHS hex as the data type of the attribute %s", map->lhs->tmpl_da->name);
+                                       return -1;
+                               }
+                               talloc_free(vpt);
+
+                       } else if (tmpl_cast_in_place(map->rhs, map->lhs->tmpl_da->type, map->lhs->tmpl_da) < 0) {
                                cf_log_err(map->ci, "%s", fr_strerror());
                                return -1;
                        }
@@ -1689,7 +1706,7 @@ static modcallable *do_compile_modupdate(modcallable *parent, rlm_components_t c
        modgroup *g;
        modcallable *csingle;
 
-       value_pair_map_t *head;
+       vp_map_t *head;
 
        /*
         *      This looks at cs->name2 to determine which list to update
@@ -2138,6 +2155,96 @@ static int all_children_are_modules(CONF_SECTION *cs, char const *name)
        return 1;
 }
 
+/** Load a named module from "instantiate" or "policy".
+ *
+ * If it's "foo.method", look for "foo", and return "method" as the method
+ * we wish to use, instead of the input component.
+ *
+ * @param[out] pcomponent Where to write the method we found, if any.  If no method is specified
+ *     will be set to MOD_COUNT.
+ * @param[in] real_name Complete name string e.g. foo.authorize.
+ * @param[in] virtual_name Virtual module name e.g. foo.
+ * @param[in] method_name Method override (may be NULL) or the method name e.g. authorize.
+ * @return the CONF_SECTION specifying the virtual module.
+ */
+static CONF_SECTION *virtual_module_find_cs(rlm_components_t *pcomponent,
+                                           char const *real_name, char const *virtual_name, char const *method_name)
+{
+       CONF_SECTION *cs, *subcs;
+       rlm_components_t method = *pcomponent;
+       char buffer[256];
+
+       /*
+        *      Turn the method name into a method enum.
+        */
+       if (method_name) {
+               rlm_components_t i;
+
+               for (i = MOD_AUTHENTICATE; i < MOD_COUNT; i++) {
+                       if (strcmp(comp2str[i], method_name) == 0) break;
+               }
+
+               if (i != MOD_COUNT) {
+                       method = i;
+               } else {
+                       method_name = NULL;
+                       virtual_name = real_name;
+               }
+       }
+
+       /*
+        *      Look for "foo" in the "instantiate" section.  If we
+        *      find it, AND there's no method name, we've found the
+        *      right thing.
+        *
+        *      Return it to the caller, with the updated method.
+        */
+       cs = cf_section_find("instantiate");
+       if (cs) {
+               /*
+                *      Found "foo".  Load it as "foo", or "foo.method".
+                */
+               subcs = cf_section_sub_find_name2(cs, NULL, virtual_name);
+               if (subcs) {
+                       *pcomponent = method;
+                       return subcs;
+               }
+       }
+
+       /*
+        *      Look for it in "policy".
+        *
+        *      If there's no policy section, we can't do anything else.
+        */
+       cs = cf_section_find("policy");
+       if (!cs) return NULL;
+
+       /*
+        *      "foo.authorize" means "load policy "foo" as method "authorize".
+        *
+        *      And bail out if there's no policy "foo".
+        */
+       if (method_name) {
+               subcs = cf_section_sub_find_name2(cs, NULL, virtual_name);
+               if (subcs) *pcomponent = method;
+
+               return subcs;
+       }
+
+       /*
+        *      "foo" means "look for foo.component" first, to allow
+        *      method overrides.  If that's not found, just look for
+        *      a policy "foo".
+        *
+        */
+       snprintf(buffer, sizeof(buffer), "%s.%s",
+                virtual_name, comp2str[method]);
+       subcs = cf_section_sub_find_name2(cs, NULL, buffer);
+       if (subcs) return subcs;
+
+       return cf_section_sub_find_name2(cs, NULL, virtual_name);
+}
+
 
 /*
  *     Compile one entry of a module call.
@@ -2414,31 +2521,21 @@ static modcallable *do_compile_modsingle(modcallable *parent,
         *      policy { ... name { .. } .. }
         *      policy { ... name.method { .. } .. }
         *
-        *      The "instantiate" virtual modules are identical to the
-        *      policies at this point.  We should probably get rid of
-        *      the "instantiate" ones, as they're duplicate and
-        *      confusing.
+        *      The only difference between things in "instantiate"
+        *      and "policy" is that "instantiate" will cause modules
+        *      to be instantiated in a particular order.
         */
        subcs = NULL;
-       cs = cf_section_find("instantiate");
-       if (cs) subcs = cf_section_sub_find_name2(cs, NULL,
-                                                 modrefname);
-       if (!subcs &&
-           (cs = cf_section_find("policy")) != NULL) {
+       p = strrchr(modrefname, '.');
+       if (!p) {
+               subcs = virtual_module_find_cs(&method, modrefname, modrefname, NULL);
+       } else {
                char buffer[256];
 
-               snprintf(buffer, sizeof(buffer), "%s.%s",
-                        modrefname, comp2str[component]);
+               strlcpy(buffer, modrefname, sizeof(buffer));
+               buffer[p - modrefname] = '\0';
 
-               /*
-                *      Prefer name.section, then name.
-                */
-               subcs = cf_section_sub_find_name2(cs, NULL,
-                                                         buffer);
-               if (!subcs) {
-                       subcs = cf_section_sub_find_name2(cs, NULL,
-                                                         modrefname);
-               }
+               subcs = virtual_module_find_cs(&method, modrefname, buffer, buffer + (p - modrefname) + 1);
        }
 
        /*
@@ -2476,7 +2573,7 @@ static modcallable *do_compile_modsingle(modcallable *parent,
                 */
                if (cf_section_name2(subcs)) {
                        csingle = do_compile_modsingle(parent,
-                                                      component,
+                                                      method,
                                                       cf_section_to_item(subcs),
                                                       grouptype,
                                                       modname);
@@ -2491,7 +2588,7 @@ static modcallable *do_compile_modsingle(modcallable *parent,
                         *      group foo { ...
                         */
                        csingle = do_compile_modgroup(parent,
-                                                     component,
+                                                     method,
                                                      subcs,
                                                      GROUPTYPE_SIMPLE,
                                                      grouptype, MOD_GROUP);
@@ -2529,63 +2626,17 @@ static modcallable *do_compile_modsingle(modcallable *parent,
                 *      modules belongs in raddb/mods-available/,
                 *      which isn't loaded into the "modules" section.
                 */
-               if (cf_section_sub_find_name2(modules, NULL, realname)) {
-                       this = find_module_instance(modules, realname, true);
-                       if (this) goto allocate_csingle;
-
-                       /*
-                        *
-                        */
-                       if (realname != modrefname) {
-                               return NULL;
-                       }
-
-               } else {
-                       /*
-                        *      We were asked to MAYBE load it and it
-                        *      doesn't exist.  Return a soft error.
-                        */
-                       if (realname != modrefname) {
-                               *modname = modrefname;
-                               return NULL;
-                       }
-               }
-       }
-
-       /*
-        *      No module found by that name.  Maybe we're calling
-        *      module.method
-        */
-       p = strrchr(modrefname, '.');
-       if (p) {
-               rlm_components_t i;
-               p++;
+               this = module_instantiate_method(modules, realname, &method);
+               if (this) goto allocate_csingle;
 
                /*
-                *      Find the component.
+                *      We were asked to MAYBE load it and it
+                *      doesn't exist.  Return a soft error.
                 */
-               for (i = RLM_COMPONENT_AUTH;
-                    i < RLM_COMPONENT_COUNT;
-                    i++) {
-                       if (strcmp(p, comp2str[i]) == 0) {
-                               char buffer[256];
-
-                               strlcpy(buffer, modrefname, sizeof(buffer));
-                               buffer[p - modrefname - 1] = '\0';
-                               component = i;
-
-                               this = find_module_instance(modules, buffer, true);
-                               if (this) {
-                                       method = i;
-                                       goto allocate_csingle;
-                               }
-                       }
+               if (realname != modrefname) {
+                       *modname = modrefname;
+                       return NULL;
                }
-
-               /*
-                *      FIXME: check for "module", and give error "no
-                *      such component" when we don't find the method.
-                */
        }
 
        /*
@@ -2618,7 +2669,7 @@ allocate_csingle:
        csingle = mod_singletocallable(single);
        csingle->parent = parent;
        csingle->next = NULL;
-       if (!parent || (component != RLM_COMPONENT_AUTH)) {
+       if (!parent || (component != MOD_AUTHENTICATE)) {
                memcpy(csingle->actions, defaultactions[component][grouptype],
                       sizeof csingle->actions);
        } else { /* inside Auth-Type has different rules */
@@ -2776,11 +2827,21 @@ static modcallable *do_compile_modgroup(modcallable *parent,
                        rad_assert(parent != NULL);
                        p = mod_callabletogroup(parent);
 
-                       rad_assert(p->tail != NULL);
+                       if (!p->tail) goto elsif_fail;
 
+                       /*
+                        *      We're in the process of compiling the
+                        *      section, so the parent's tail is the
+                        *      previous "if" statement.
+                        */
                        f = mod_callabletogroup(p->tail);
-                       rad_assert((f->mc.type == MOD_IF) ||
-                                  (f->mc.type == MOD_ELSIF));
+                       if ((f->mc.type != MOD_IF) &&
+                           (f->mc.type != MOD_ELSIF)) {
+                       elsif_fail:
+                               cf_log_err_cs(g->cs, "Invalid location for 'elsif'.  There is no preceding 'if' statement");
+                               talloc_free(g);
+                               return NULL;
+                       }
 
                        /*
                         *      If we took the previous condition, we
@@ -2807,11 +2868,16 @@ static modcallable *do_compile_modgroup(modcallable *parent,
                        rad_assert(parent != NULL);
                        p = mod_callabletogroup(parent);
 
-                       rad_assert(p->tail != NULL);
+                       if (!p->tail) goto else_fail;
 
                        f = mod_callabletogroup(p->tail);
-                       rad_assert((f->mc.type == MOD_IF) ||
-                                  (f->mc.type == MOD_ELSIF));
+                       if ((f->mc.type != MOD_IF) &&
+                           (f->mc.type != MOD_ELSIF)) {
+                       else_fail:
+                               cf_log_err_cs(g->cs, "Invalid location for 'else'.  There is no preceding 'if' statement");
+                               talloc_free(g);
+                               return NULL;
+                       }
 
                        /*
                         *      If we took the previous condition, we
@@ -2907,7 +2973,7 @@ set_codes:
         */
        for (i = 0; i < RLM_MODULE_NUMCODES; i++) {
                if (!c->actions[i]) {
-                       if (!parent || (component != RLM_COMPONENT_AUTH)) {
+                       if (!parent || (component != MOD_AUTHENTICATE)) {
                                c->actions[i] = defaultactions[component][parentgrouptype][i];
                        } else { /* inside Auth-Type has different rules */
                                c->actions[i] = authtype_actions[parentgrouptype][i];
@@ -2946,7 +3012,7 @@ modcallable *compile_modgroup(modcallable *parent,
                                               GROUPTYPE_SIMPLE,
                                               GROUPTYPE_SIMPLE, MOD_GROUP);
 
-       if (debug_flag > 3) {
+       if (rad_debug_lvl > 3) {
                modcall_debug(ret, 2);
        }
 
@@ -3026,13 +3092,13 @@ static bool pass2_xlat_compile(CONF_ITEM const *ci, vp_tmpl_t **pvpt, bool conve
                        if (cf_item_is_pair(ci)) {
                                CONF_PAIR *cp = cf_item_to_pair(ci);
 
-                               WARN("%s[%d] Please change %%{%s} to &%s",
+                               WARN("%s[%d]: Please change \"%%{%s}\" to &%s",
                                       cf_pair_filename(cp), cf_pair_lineno(cp),
                                       attr->name, attr->name);
                        } else {
                                CONF_SECTION *cs = cf_item_to_section(ci);
 
-                               WARN("%s[%d] Please change %%{%s} to &%s",
+                               WARN("%s[%d]: Please change \"%%{%s}\" to &%s",
                                       cf_section_filename(cs), cf_section_lineno(cs),
                                       attr->name, attr->name);
                        }
@@ -3117,10 +3183,27 @@ static bool pass2_fixup_undefined(CONF_ITEM const *ci, vp_tmpl_t *vpt)
        return true;
 }
 
-static bool pass2_callback(UNUSED void *ctx, fr_cond_t *c)
+static bool pass2_callback(void *ctx, fr_cond_t *c)
 {
-       value_pair_map_t *map;
+       vp_map_t *map;
+       vp_tmpl_t *vpt;
+
+       /*
+        *      These don't get optimized.
+        */
+       if ((c->type == COND_TYPE_TRUE) ||
+           (c->type == COND_TYPE_FALSE)) {
+               return true;
+       }
 
+       /*
+        *      Call children.
+        */
+       if (c->type == COND_TYPE_CHILD) return pass2_callback(ctx, c->data.child);
+
+       /*
+        *      A few simple checks here.
+        */
        if (c->type == COND_TYPE_EXISTS) {
                if (c->data.vpt->type == TMPL_TYPE_XLAT) {
                        return pass2_xlat_compile(c->ci, &c->data.vpt, true, NULL);
@@ -3136,22 +3219,24 @@ static bool pass2_callback(UNUSED void *ctx, fr_cond_t *c)
                        if (!pass2_fixup_undefined(c->ci, c->data.vpt)) return false;
                        c->pass2_fixup = PASS2_FIXUP_NONE;
                }
-               return true;
-       }
 
-       /*
-        *      Maps have a paircompare fixup applied to them.
-        *      Others get ignored.
-        */
-       if (c->pass2_fixup == PASS2_FIXUP_NONE) {
-               if (c->type == COND_TYPE_MAP) {
-                       map = c->data.map;
-                       goto check_paircmp;
+               /*
+                *      Convert virtual &Attr-Foo to "%{Attr-Foo}"
+                */
+               vpt = c->data.vpt;
+               if ((vpt->type == TMPL_TYPE_ATTR) && vpt->tmpl_da->flags.virtual) {
+                       vpt->tmpl_xlat = xlat_from_tmpl_attr(vpt, vpt);
+                       vpt->type = TMPL_TYPE_XLAT_STRUCT;
                }
 
                return true;
        }
 
+       /*
+        *      And tons of complicated checks.
+        */
+       rad_assert(c->type == COND_TYPE_MAP);
+
        map = c->data.map;      /* shorter */
 
        /*
@@ -3188,7 +3273,6 @@ static bool pass2_callback(UNUSED void *ctx, fr_cond_t *c)
                c->pass2_fixup = PASS2_FIXUP_NONE;
        }
 
-check_paircmp:
        /*
         *      Just in case someone adds a new fixup later.
         */
@@ -3200,13 +3284,88 @@ check_paircmp:
         */
        if (map->lhs->type == TMPL_TYPE_XLAT) {
                /*
-                *      Don't compile the LHS to an attribute
-                *      reference for now.  When we do that, we've got
-                *      to check the RHS for type-specific data, and
-                *      parse it to a TMPL_TYPE_DATA.
+                *      Compile the LHS to an attribute reference only
+                *      if the RHS is a literal.
+                *
+                *      @todo v3.1: allow anything anywhere.
                 */
-               if (!pass2_xlat_compile(map->ci, &map->lhs, false, NULL)) {
-                       return false;
+               if (map->rhs->type != TMPL_TYPE_LITERAL) {
+                       if (!pass2_xlat_compile(map->ci, &map->lhs, false, NULL)) {
+                               return false;
+                       }
+               } else {
+                       if (!pass2_xlat_compile(map->ci, &map->lhs, true, NULL)) {
+                               return false;
+                       }
+
+                       /*
+                        *      Attribute compared to a literal gets
+                        *      the literal cast to the data type of
+                        *      the attribute.
+                        *
+                        *      The code in parser.c did this for
+                        *
+                        *              &Attr == data
+                        *
+                        *      But now we've just converted "%{Attr}"
+                        *      to &Attr, so we've got to do it again.
+                        */
+                       if ((map->lhs->type == TMPL_TYPE_ATTR) &&
+                           (map->rhs->type == TMPL_TYPE_LITERAL)) {
+                               /*
+                                *      RHS is hex, try to parse it as
+                                *      type-specific data.
+                                */
+                               if (map->lhs->auto_converted &&
+                                   (map->rhs->name[0] == '0') && (map->rhs->name[1] == 'x') &&
+                                   (map->rhs->len > 2) && ((map->rhs->len & 0x01) == 0)) {
+                                       vpt = map->rhs;
+                                       map->rhs = NULL;
+
+                                       if (!map_cast_from_hex(map, T_BARE_WORD, vpt->name)) {
+                                               map->rhs = vpt;
+                                               cf_log_err(map->ci, "Cannot parse RHS hex as the data type of the attribute %s", map->lhs->tmpl_da->name);
+                                               return -1;
+                                       }
+                                       talloc_free(vpt);
+
+                               } else if ((map->rhs->len > 0) ||
+                                          (map->op != T_OP_CMP_EQ) ||
+                                          (map->lhs->tmpl_da->type == PW_TYPE_STRING) ||
+                                          (map->lhs->tmpl_da->type == PW_TYPE_OCTETS)) {
+
+                                       if (tmpl_cast_in_place(map->rhs, map->lhs->tmpl_da->type, map->lhs->tmpl_da) < 0) {
+                                               cf_log_err(map->ci, "Failed to parse data type %s from string: %s",
+                                                          fr_int2str(dict_attr_types, map->lhs->tmpl_da->type, "<UNKNOWN>"),
+                                                          map->rhs->name);
+                                               return false;
+                                       } /* else the cast was successful */
+
+                               } else {        /* RHS is empty, it's just a check for empty / non-empty string */
+                                       vpt = talloc_steal(c, map->lhs);
+                                       map->lhs = NULL;
+                                       talloc_free(c->data.map);
+
+                                       /*
+                                        *      "%{Foo}" == '' ---> !Foo
+                                        *      "%{Foo}" != '' ---> Foo
+                                        */
+                                       c->type = COND_TYPE_EXISTS;
+                                       c->data.vpt = vpt;
+                                       c->negate = !c->negate;
+
+                                       WARN("%s[%d]: Please change (\"%%{%s}\" %s '') to %c&%s",
+                                            cf_section_filename(cf_item_to_section(c->ci)),
+                                            cf_section_lineno(cf_item_to_section(c->ci)),
+                                            vpt->name, c->negate ? "==" : "!=",
+                                            c->negate ? '!' : ' ', vpt->name);
+
+                                       /*
+                                        *      No more RHS, so we can't do more optimizations
+                                        */
+                                       return true;
+                               }
+                       }
                }
        }
 
@@ -3244,7 +3403,6 @@ check_paircmp:
            (strncmp(map->lhs->name, "Foreach-Variable-", 17) == 0)) {
                char *fmt;
                ssize_t slen;
-               vp_tmpl_t *vpt;
 
                fmt = talloc_asprintf(map->lhs, "%%{%s}", map->lhs->name);
                slen = tmpl_afrom_str(map, &vpt, fmt, talloc_array_length(fmt) - 1,
@@ -3278,6 +3436,32 @@ check_paircmp:
 #endif
 
        /*
+        *      Convert &Packet-Type to "%{Packet-Type}", because
+        *      these attributes don't really exist.  The code to
+        *      find an attribute reference doesn't work, but the
+        *      xlat code does.
+        */
+       vpt = c->data.map->lhs;
+       if ((vpt->type == TMPL_TYPE_ATTR) && vpt->tmpl_da->flags.virtual) {
+               if (!c->cast) c->cast = vpt->tmpl_da;
+               vpt->tmpl_xlat = xlat_from_tmpl_attr(vpt, vpt);
+               vpt->type = TMPL_TYPE_XLAT_STRUCT;
+       }
+
+       /*
+        *      Convert RHS to expansions, too.
+        */
+       vpt = c->data.map->rhs;
+       if ((vpt->type == TMPL_TYPE_ATTR) && vpt->tmpl_da->flags.virtual) {
+               vpt->tmpl_xlat = xlat_from_tmpl_attr(vpt, vpt);
+               vpt->type = TMPL_TYPE_XLAT_STRUCT;
+       }
+
+       /*
+        *      @todo v3.1: do the same thing for the RHS...
+        */
+
+       /*
         *      Only attributes can have a paircompare registered, and
         *      they can only be with the current REQUEST, and only
         *      with the request pairs.
@@ -3316,7 +3500,7 @@ check_paircmp:
 
        /*
         *      Mark it as requiring a paircompare() call, instead of
-        *      paircmp().
+        *      fr_pair_cmp().
         */
        c->pass2_fixup = PASS2_PAIRCOMPARE;
 
@@ -3329,7 +3513,7 @@ check_paircmp:
  */
 static bool modcall_pass2_update(modgroup *g)
 {
-       value_pair_map_t *map;
+       vp_map_t *map;
 
        for (map = g->map; map != NULL; map = map->next) {
                if (map->rhs->type == TMPL_TYPE_XLAT) {
@@ -3486,6 +3670,14 @@ bool modcall_pass2(modcallable *mc)
                        }
 
                        /*
+                        *      Convert virtual &Attr-Foo to "%{Attr-Foo}"
+                        */
+                       if ((g->vpt->type == TMPL_TYPE_ATTR) && g->vpt->tmpl_da->flags.virtual) {
+                               g->vpt->tmpl_xlat = xlat_from_tmpl_attr(g->vpt, g->vpt);
+                               g->vpt->type = TMPL_TYPE_XLAT_STRUCT;
+                       }
+
+                       /*
                         *      We may have: switch Foo-Bar {
                         *
                         *      where Foo-Bar is an attribute defined
@@ -3517,9 +3709,9 @@ bool modcall_pass2(modcallable *mc)
                        if ((g->vpt->type == TMPL_TYPE_ATTR) &&
                            (c->name[0] != '&')) {
                                WARN("%s[%d]: Please change %s to &%s",
-                                      cf_section_filename(g->cs),
-                                      cf_section_lineno(g->cs),
-                                      c->name, c->name);
+                                    cf_section_filename(g->cs),
+                                    cf_section_lineno(g->cs),
+                                    c->name, c->name);
                        }
 
                do_children:
@@ -3590,6 +3782,12 @@ bool modcall_pass2(modcallable *mc)
                                goto do_children;
                        }
 
+                       if (g->vpt->type == TMPL_TYPE_ATTR_UNDEFINED) {
+                               if (!pass2_fixup_undefined(cf_section_to_item(g->cs), g->vpt)) {
+                                       return false;
+                               }
+                       }
+
                        /*
                         *      Compile and sanity check xlat
                         *      expansions.
@@ -3617,6 +3815,14 @@ bool modcall_pass2(modcallable *mc)
                                }
                        }
 
+                       /*
+                        *      Virtual attribute fixes for "case" statements, too.
+                        */
+                       if ((g->vpt->type == TMPL_TYPE_ATTR) && g->vpt->tmpl_da->flags.virtual) {
+                               g->vpt->tmpl_xlat = xlat_from_tmpl_attr(g->vpt, g->vpt);
+                               g->vpt->type = TMPL_TYPE_XLAT_STRUCT;
+                       }
+
                        if (!modcall_pass2(g->children)) return false;
                        g->done_pass2 = true;
                        break;
@@ -3687,7 +3893,13 @@ bool modcall_pass2(modcallable *mc)
                                char const *name1 = cf_section_name1(g->cs);
 
                                if (strcmp(name1, unlang_keyword[c->type]) != 0) {
-                                       c->debug_name = talloc_asprintf(c, "%s %s", name1, cf_section_name2(g->cs));
+                                       name2 = cf_section_name2(g->cs);
+
+                                       if (!name2) {
+                                               c->debug_name = name1;
+                                       } else {
+                                               c->debug_name = talloc_asprintf(c, "%s %s", name1, name2);
+                                       }
                                }
                        }
 
@@ -3708,7 +3920,7 @@ void modcall_debug(modcallable *mc, int depth)
 {
        modcallable *this;
        modgroup *g;
-       value_pair_map_t *map;
+       vp_map_t *map;
        char buffer[1024];
 
        for (this = mc; this != NULL; this = this->next) {
@@ -3800,3 +4012,10 @@ void modcall_debug(modcallable *mc, int depth)
                }
        }
 }
+
+int modcall_pass2_condition(fr_cond_t *c)
+{
+       if (!fr_condition_walk(c, pass2_callback, NULL)) return -1;
+
+       return 0;
+}