Re: [Xen-devel] [PATCH v2] xen/vsprintf: Introduce %pd formatter for domains
>>> On 01.10.18 at 15:55, wrote:
> On 01/10/18 11:25, Jan Beulich wrote:
> On 01.10.18 at 12:23, wrote:
>>> On 01/10/18 11:14, Jan Beulich wrote:
>>> On 01.10.18 at 12:02, wrote:
> On 01/10/18 10:08, Jan Beulich wrote:
> On 28.09.18 at 19:22, wrote:
>>> +static char *print_domain(char *str, char *end, const struct domain *d)
>>> +{
>>> +const char *name = NULL;
>>> +
>>> +/* Some debugging may have an optionally-NULL pointer. */
>>> +if ( unlikely(!d) )
>>> +return string(str, end, "NULL", -1, -1, 0);
>>> +
>>> +if ( str < end )
>>> +*str = 'd';
>>> +
>>> +switch ( d->domain_id )
>>> +{
>>> +case DOMID_IO: name = "[IO]"; break;
>>> +case DOMID_XEN: name = "[XEN]"; break;
>>> +case DOMID_COW: name = "[COW]"; break;
>>> +case DOMID_IDLE: name = "[IDLE]"; break;
>> default: ASSERT_UNREACHABLE();
>>
>> ?
> No - specifically not in this case.
>
> This path is used when printing crash information, and falling back to a
> number is better behaviour than falling into an infinite loop,
> overflowing the primary stack, then taking a #DF (which escalates to
> triple fault on AMD), without printing anything useful.
Ah, good point. Perhaps worth a brief comment instead of a "default:"
then?
>>> This incremental diff?
>> LGTM, thanks.
>
> I've committed this now, but its just occurred to me that we couldn't
> possibly have had a default case, because that is the common case for
> most domains.
Oh, indeed, but still, while the "unreachable" part of my suggestion
was indeed wrong, but it still could have been
ASSERT(d->domain_id < DOMID_FIRST_RESERVED). But I agree this
wouldn't have been overly helpful.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] xen/vsprintf: Introduce %pd formatter for domains
On 01/10/18 11:25, Jan Beulich wrote:
On 01.10.18 at 12:23, wrote:
>> On 01/10/18 11:14, Jan Beulich wrote:
>> On 01.10.18 at 12:02, wrote:
On 01/10/18 10:08, Jan Beulich wrote:
On 28.09.18 at 19:22, wrote:
>> +static char *print_domain(char *str, char *end, const struct domain *d)
>> +{
>> +const char *name = NULL;
>> +
>> +/* Some debugging may have an optionally-NULL pointer. */
>> +if ( unlikely(!d) )
>> +return string(str, end, "NULL", -1, -1, 0);
>> +
>> +if ( str < end )
>> +*str = 'd';
>> +
>> +switch ( d->domain_id )
>> +{
>> +case DOMID_IO: name = "[IO]"; break;
>> +case DOMID_XEN: name = "[XEN]"; break;
>> +case DOMID_COW: name = "[COW]"; break;
>> +case DOMID_IDLE: name = "[IDLE]"; break;
> default: ASSERT_UNREACHABLE();
>
> ?
No - specifically not in this case.
This path is used when printing crash information, and falling back to a
number is better behaviour than falling into an infinite loop,
overflowing the primary stack, then taking a #DF (which escalates to
triple fault on AMD), without printing anything useful.
>>> Ah, good point. Perhaps worth a brief comment instead of a "default:"
>>> then?
>> This incremental diff?
> LGTM, thanks.
I've committed this now, but its just occurred to me that we couldn't
possibly have had a default case, because that is the common case for
most domains.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] xen/vsprintf: Introduce %pd formatter for domains
>>> On 01.10.18 at 12:23, wrote:
> On 01/10/18 11:14, Jan Beulich wrote:
> On 01.10.18 at 12:02, wrote:
>>> On 01/10/18 10:08, Jan Beulich wrote:
>>> On 28.09.18 at 19:22, wrote:
> +static char *print_domain(char *str, char *end, const struct domain *d)
> +{
> +const char *name = NULL;
> +
> +/* Some debugging may have an optionally-NULL pointer. */
> +if ( unlikely(!d) )
> +return string(str, end, "NULL", -1, -1, 0);
> +
> +if ( str < end )
> +*str = 'd';
> +
> +switch ( d->domain_id )
> +{
> +case DOMID_IO: name = "[IO]"; break;
> +case DOMID_XEN: name = "[XEN]"; break;
> +case DOMID_COW: name = "[COW]"; break;
> +case DOMID_IDLE: name = "[IDLE]"; break;
default: ASSERT_UNREACHABLE();
?
>>> No - specifically not in this case.
>>>
>>> This path is used when printing crash information, and falling back to a
>>> number is better behaviour than falling into an infinite loop,
>>> overflowing the primary stack, then taking a #DF (which escalates to
>>> triple fault on AMD), without printing anything useful.
>> Ah, good point. Perhaps worth a brief comment instead of a "default:"
>> then?
>
> This incremental diff?
LGTM, thanks.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] xen/vsprintf: Introduce %pd formatter for domains
On 01/10/18 11:14, Jan Beulich wrote:
On 01.10.18 at 12:02, wrote:
>> On 01/10/18 10:08, Jan Beulich wrote:
>> On 28.09.18 at 19:22, wrote:
+static char *print_domain(char *str, char *end, const struct domain *d)
+{
+const char *name = NULL;
+
+/* Some debugging may have an optionally-NULL pointer. */
+if ( unlikely(!d) )
+return string(str, end, "NULL", -1, -1, 0);
+
+if ( str < end )
+*str = 'd';
+
+switch ( d->domain_id )
+{
+case DOMID_IO: name = "[IO]"; break;
+case DOMID_XEN: name = "[XEN]"; break;
+case DOMID_COW: name = "[COW]"; break;
+case DOMID_IDLE: name = "[IDLE]"; break;
>>> default: ASSERT_UNREACHABLE();
>>>
>>> ?
>> No - specifically not in this case.
>>
>> This path is used when printing crash information, and falling back to a
>> number is better behaviour than falling into an infinite loop,
>> overflowing the primary stack, then taking a #DF (which escalates to
>> triple fault on AMD), without printing anything useful.
> Ah, good point. Perhaps worth a brief comment instead of a "default:"
> then?
This incremental diff?
diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
index cd2d7d9..b0ff00c 100644
--- a/xen/common/vsprintf.c
+++ b/xen/common/vsprintf.c
@@ -279,6 +279,11 @@ static char *print_domain(char *str, char *end,
const struct domain *d)
case DOMID_XEN: name = "[XEN]"; break;
case DOMID_COW: name = "[COW]"; break;
case DOMID_IDLE: name = "[IDLE]"; break;
+ /*
+ * In principle, we could ASSERT_UNREACHABLE() in the default case.
+ * However, this path is used to print out crash information, which
+ * risks recursing infinitely and not printing any useful
information.
+ */
}
if ( str < end )
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] xen/vsprintf: Introduce %pd formatter for domains
>>> On 01.10.18 at 12:02, wrote:
> On 01/10/18 10:08, Jan Beulich wrote:
> On 28.09.18 at 19:22, wrote:
>>> +static char *print_domain(char *str, char *end, const struct domain *d)
>>> +{
>>> +const char *name = NULL;
>>> +
>>> +/* Some debugging may have an optionally-NULL pointer. */
>>> +if ( unlikely(!d) )
>>> +return string(str, end, "NULL", -1, -1, 0);
>>> +
>>> +if ( str < end )
>>> +*str = 'd';
>>> +
>>> +switch ( d->domain_id )
>>> +{
>>> +case DOMID_IO: name = "[IO]"; break;
>>> +case DOMID_XEN: name = "[XEN]"; break;
>>> +case DOMID_COW: name = "[COW]"; break;
>>> +case DOMID_IDLE: name = "[IDLE]"; break;
>> default: ASSERT_UNREACHABLE();
>>
>> ?
>
> No - specifically not in this case.
>
> This path is used when printing crash information, and falling back to a
> number is better behaviour than falling into an infinite loop,
> overflowing the primary stack, then taking a #DF (which escalates to
> triple fault on AMD), without printing anything useful.
Ah, good point. Perhaps worth a brief comment instead of a "default:"
then?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] xen/vsprintf: Introduce %pd formatter for domains
On 01/10/18 10:08, Jan Beulich wrote:
On 28.09.18 at 19:22, wrote:
>> --- a/xen/common/vsprintf.c
>> +++ b/xen/common/vsprintf.c
>> @@ -264,6 +264,47 @@ static char *string(char *str, char *end, const char *s,
>> return str;
>> }
>>
>> +/* Print a domain id, using names for system domains. (e.g. d0 or d[IDLE])
>> */
>> +static char *print_domain(char *str, char *end, const struct domain *d)
>> +{
>> +const char *name = NULL;
>> +
>> +/* Some debugging may have an optionally-NULL pointer. */
>> +if ( unlikely(!d) )
>> +return string(str, end, "NULL", -1, -1, 0);
>> +
>> +if ( str < end )
>> +*str = 'd';
>> +
>> +switch ( d->domain_id )
>> +{
>> +case DOMID_IO: name = "[IO]"; break;
>> +case DOMID_XEN: name = "[XEN]"; break;
>> +case DOMID_COW: name = "[COW]"; break;
>> +case DOMID_IDLE: name = "[IDLE]"; break;
> default: ASSERT_UNREACHABLE();
>
> ?
No - specifically not in this case.
This path is used when printing crash information, and falling back to a
number is better behaviour than falling into an infinite loop,
overflowing the primary stack, then taking a #DF (which escalates to
triple fault on AMD), without printing anything useful.
In general, we cannot have BUG/WARN/ASSERTs in the middle vsprintf() path.
>
>> +}
> Perhaps the insertion of 'd' might better live here, to make a
> better connection between the lack of a ++ there and the + 1
> below?
Will move.
>
>> +if ( name )
>> +return string(str + 1, end, name, -1, -1, 0);
>> +else
>> +return number(str + 1, end, d->domain_id, 10, -1, -1, 0);
>> +}
> Anyway, even without the adjustments
> Reviewed-by: Jan Beulich
Thanks,
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] xen/vsprintf: Introduce %pd formatter for domains
>>> On 28.09.18 at 19:22, wrote:
> --- a/xen/common/vsprintf.c
> +++ b/xen/common/vsprintf.c
> @@ -264,6 +264,47 @@ static char *string(char *str, char *end, const char *s,
> return str;
> }
>
> +/* Print a domain id, using names for system domains. (e.g. d0 or d[IDLE])
> */
> +static char *print_domain(char *str, char *end, const struct domain *d)
> +{
> +const char *name = NULL;
> +
> +/* Some debugging may have an optionally-NULL pointer. */
> +if ( unlikely(!d) )
> +return string(str, end, "NULL", -1, -1, 0);
> +
> +if ( str < end )
> +*str = 'd';
> +
> +switch ( d->domain_id )
> +{
> +case DOMID_IO: name = "[IO]"; break;
> +case DOMID_XEN: name = "[XEN]"; break;
> +case DOMID_COW: name = "[COW]"; break;
> +case DOMID_IDLE: name = "[IDLE]"; break;
default: ASSERT_UNREACHABLE();
?
> +}
Perhaps the insertion of 'd' might better live here, to make a
better connection between the lack of a ++ there and the + 1
below?
> +if ( name )
> +return string(str + 1, end, name, -1, -1, 0);
> +else
> +return number(str + 1, end, d->domain_id, 10, -1, -1, 0);
> +}
Anyway, even without the adjustments
Reviewed-by: Jan Beulich
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2] xen/vsprintf: Introduce %pd formatter for domains
This allows all system domids to be printed by name, rather than special
casing the idle vcpus alone.
Signed-off-by: Andrew Cooper
---
CC: George Dunlap
CC: Jan Beulich
CC: Konrad Rzeszutek Wilk
CC: Stefano Stabellini
CC: Tim Deegan
CC: Wei Liu
CC: Julien Grall
v2:
* Render system names in square brackets.
* Drop DOMID_{SELF,INVALID} names because there are no struct domain *'s for
them.
* Cope with NULL pointers.
* Fix a length-counting bug.
---
docs/misc/printk-formats.txt | 14 --
xen/common/vsprintf.c| 61 +---
2 files changed, 58 insertions(+), 17 deletions(-)
diff --git a/docs/misc/printk-formats.txt b/docs/misc/printk-formats.txt
index 525108f..b5570bc 100644
--- a/docs/misc/printk-formats.txt
+++ b/docs/misc/printk-formats.txt
@@ -28,5 +28,15 @@ Symbol/Function pointers:
Domain and vCPU information:
- %pv Domain and vCPU ID from a 'struct vcpu *' (printed as
- "dv")
+ %pd Domain from a 'struct domain *'
+
+ Regular domains are printed with their ID in decimal. System
+ domains are printed with their name.
+ e.g. d0
+ d[IDLE]
+
+ %pv Domain and vCPU ID from a 'struct vcpu *'
+
+ The domain part as above, with the vcpu_id printed in decimal.
+ e.g. d0v1
+ d[IDLE]v0
diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
index f92fb67..df347d3 100644
--- a/xen/common/vsprintf.c
+++ b/xen/common/vsprintf.c
@@ -264,6 +264,47 @@ static char *string(char *str, char *end, const char *s,
return str;
}
+/* Print a domain id, using names for system domains. (e.g. d0 or d[IDLE]) */
+static char *print_domain(char *str, char *end, const struct domain *d)
+{
+const char *name = NULL;
+
+/* Some debugging may have an optionally-NULL pointer. */
+if ( unlikely(!d) )
+return string(str, end, "NULL", -1, -1, 0);
+
+if ( str < end )
+*str = 'd';
+
+switch ( d->domain_id )
+{
+case DOMID_IO: name = "[IO]"; break;
+case DOMID_XEN: name = "[XEN]"; break;
+case DOMID_COW: name = "[COW]"; break;
+case DOMID_IDLE: name = "[IDLE]"; break;
+}
+
+if ( name )
+return string(str + 1, end, name, -1, -1, 0);
+else
+return number(str + 1, end, d->domain_id, 10, -1, -1, 0);
+}
+
+/* Print a vcpu id. (e.g. d0v1 or d[IDLE]v0) */
+static char *print_vcpu(char *str, char *end, const struct vcpu *v)
+{
+/* Some debugging may have an optionally-NULL pointer. */
+if ( unlikely(!v) )
+return string(str, end, "NULL", -1, -1, 0);
+
+str = print_domain(str, end, v->domain);
+
+if ( str < end )
+*str = 'v';
+
+return number(str + 1, end, v->vcpu_id, 10, -1, -1, 0);
+}
+
static char *pointer(char *str, char *end, const char **fmt_ptr,
const void *arg, int field_width, int precision,
int flags)
@@ -273,6 +314,10 @@ static char *pointer(char *str, char *end, const char
**fmt_ptr,
/* Custom %p suffixes. See XEN_ROOT/docs/misc/printk-formats.txt */
switch ( fmt[1] )
{
+case 'd': /* Domain ID from a struct domain *. */
+++*fmt_ptr;
+return print_domain(str, end, arg);
+
case 'h': /* Raw buffer as hex string. */
{
const uint8_t *hex_buffer = arg;
@@ -370,22 +415,8 @@ static char *pointer(char *str, char *end, const char
**fmt_ptr,
}
case 'v': /* dv from a struct vcpu */
-{
-const struct vcpu *v = arg;
-
++*fmt_ptr;
-if ( unlikely(v->domain->domain_id == DOMID_IDLE) )
-str = string(str, end, "IDLE", -1, -1, 0);
-else
-{
-if ( str < end )
-*str = 'd';
-str = number(str + 1, end, v->domain->domain_id, 10, -1, -1, 0);
-}
-if ( str < end )
-*str = 'v';
-return number(str + 1, end, v->vcpu_id, 10, -1, -1, 0);
-}
+return print_vcpu(str, end, arg);
}
if ( field_width == -1 )
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
