Re: [Crash-utility] [PATCH 0/7] Upgrade the remote access to a xen domain.

----- Original Message ----- Hi Don, A few comments... I don't really understand what entity the code is communicating with, so for the most part, I'm primarily interested in making sure that your patches in no way can affect the normal usage of the crash utility. That being said, is the term "NIL mode" some kind of Xen-ism? It wasn't until I saw the display_sys_stats() function that I even understood that REMOTE_NIL mean "(remote paused system)". Anyway, if you made it something like "REMOTE_PAUSED" or something like that, the code might make some sense to the untrained eye. And it's not entirely clear to me what REMOTE_ACTIVE() means now? It used to imply communicating with a remote crash daeamon that accessed the live system that the daemon was running on. Has that meaning been changed to mean that crash is communicating with a daemon that's accessing a "live" Xen guest kernel? I'm confused about these two changes, where you re-patch your own patch: - else + else if (REMOTE_ACTIVE() && ((bt->task == tt->this_task) || is_task_active(bt->task))) { + if (get_remote_regs(bt, &eip, &esp)) + machdep->get_stack_frame(bt, &eip, &esp); + } else followed by: - else if (REMOTE_ACTIVE() && ((bt->task == tt->this_task) || is_task_active(bt->task))) { + else if (REMOTE_NIL() && ((bt->task == tt->this_task) || is_task_active(bt->task))) { if (get_remote_regs(bt, &eip, &esp)) machdep->get_stack_frame(bt, &eip, &esp); } else If I'm not mistaken, it would seem to be impossible for "(bt->task == tt->this_task)" to ever be true in the REMOTE_NIL() case? As I recall tt->this_task used to be the PID of the remote daemon itself, which would be accessing the memory of its own kernel. So how could the pid of the remote daemon ever be the target of a back trace of a paused guest? And so I don't understand why you first made an if clause for REMOTE_ACTIVE() to begin with -- whatever that means now -- and then changed it to REMOTE_NIL()? In any case, since the ultimate change is only interested in REMOTE_NIL(), can you make it all less of an eyesore by encapulating its if case something like: else if (SADUMP_DUMPFILE()) get_sadump_regs(bt, &eip, &esp); + else if (REMOTE_NIL()) { + if (!((bt->task == tt->this_task) || is_task_active(bt->task)) || + !get_remote_regs(bt, &eip, &esp)) + machdep->get_stack_frame(bt, &eip, &esp); } else machdep->get_stack_frame(bt, &eip, &esp); Note above, for consistency's sake with the rest of the crash utility, can you make get_remote_regs() return non-zero when it successfully gets the registers, and zero if it fails? And lastly, can you fix these? $ make warn ... remote.c: In function ‘validate_phys_base’: remote.c:2302:25: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 3 has type ‘physaddr_t’ [-Wformat] remote.c:2302:25: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 4 has type ‘physaddr_t’ [-Wformat] remote.c:2302:25: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 5 has type ‘physaddr_t’ [-Wformat] remote.c:2302:25: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 6 has type ‘physaddr_t’ [-Wformat] remote.c: In function ‘remote_vtop’: remote.c:2361:2: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 4 has type ‘physaddr_t’ [-Wformat] remote.c:2367:4: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 4 has type ‘physaddr_t’ [-Wformat] remote.c:2352:18: warning: variable ‘p3’ set but not used [-Wunused-but-set-variable] remote.c:2352:13: warning: variable ‘p2’ set but not used [-Wunused-but-set-variable] remote.c:2352:8: warning: variable ‘p1’ set but not used [-Wunused-but-set-variable] remote.c: In function ‘get_remote_regs’: remote.c:2392:33: warning: unused variable ‘p6’ [-Wunused-variable] remote.c:2392:28: warning: variable ‘p5’ set but not used [-Wunused-but-set-variable] remote.c:2392:18: warning: variable ‘p3’ set but not used [-Wunused-but-set-variable] remote.c:2392:8: warning: variable ‘p1’ set but not used [-Wunused-but-set-variable] remote.c: In function ‘get_remote_cr3’: remote.c:2449:13: warning: variable ‘p2’ set but not used [-Wunused-but-set-variable] remote.c:2449:8: warning: variable ‘p1’ set but not used [-Wunused-but-set-variable] ... And I'm not interested in appending these to the 4 files that you changed: +/* + * Specify Emacs local variables so the formating + * of the code stays the same. + * + * Local variables: + * mode: C + * c-set-style: "BSD" + * c-basic-offset: 8 + * indent-tabs-mode: t + * End: + */ Thanks, Dave