Re: [Xen-devel] [PATCH 2/2] x86/boot: rename send_chr to print_err

2017-10-16 Thread Jan Beulich 
>>> On 13.10.17 at 18:07,  wrote:
> On 10/13/17 2:40 AM, Jan Beulich wrote:
> On 12.10.17 at 22:56,  wrote:
>>> On 12/10/2017 21:50, Doug Goldstein wrote:
 From: David Esler 

 The send_chr function sends an entire C-string and not one character and
 doesn't necessarily just send it over the serial UART anymore so rename
 it to print_err so that its closer in name to what it does.

 Reviewed-by: Doug Goldstein 
 Signed-off-by: David Esler 
>>>
>>> Reviewed-by: Andrew Cooper 
>>>
>>> This should also be included in 4.10 IMO.
>> 
>> I'm not convinced - this is merely a cleanup style patch, the more
>> that the label really serves dual purpose and hence the original
>> name isn't all that wrong anyway.
> 
> I purposefully broke it out so that it could be discussed. Prior to the
> commit I referenced in patch 1 the function sent out a character. Now it
> requires the supplied data to be a C-string (NULL terminated) so I would
> hope that you could agree that at else the "chr" part of "send_chr" is
> incorrect and should likely be "str" or "err".
> 
> But again this patch isn't really important its more to try to make the
> label match the behavior of the code below the label and if its unwanted
> then it can be dropped.

Oh, in case my earlier reply was ambiguous: I didn't mean to
object to the patch. I'm just unconvinced it should go into 4.10.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 2/2] x86/boot: rename send_chr to print_err

2017-10-13 Thread Andrew Cooper 
On 13/10/17 17:07, Doug Goldstein wrote:
> On 10/13/17 2:40 AM, Jan Beulich wrote:
> On 12.10.17 at 22:56,  wrote:
>>> On 12/10/2017 21:50, Doug Goldstein wrote:
 From: David Esler 

 The send_chr function sends an entire C-string and not one character and
 doesn't necessarily just send it over the serial UART anymore so rename
 it to print_err so that its closer in name to what it does.

 Reviewed-by: Doug Goldstein 
 Signed-off-by: David Esler 
>>> Reviewed-by: Andrew Cooper 
>>>
>>> This should also be included in 4.10 IMO.
>> I'm not convinced - this is merely a cleanup style patch, the more
>> that the label really serves dual purpose and hence the original
>> name isn't all that wrong anyway.
>>
>> Jan
>>
> I purposefully broke it out so that it could be discussed. Prior to the
> commit I referenced in patch 1 the function sent out a character. Now it
> requires the supplied data to be a C-string (NULL terminated) so I would
> hope that you could agree that at else the "chr" part of "send_chr" is
> incorrect and should likely be "str" or "err".
>
> But again this patch isn't really important its more to try to make the
> label match the behavior of the code below the label and if its unwanted
> then it can be dropped.
>

We've not hit RC1 yet, and this is fixing a misleading bit of
documentation in a hard-to-follow piece of code.

I'm still of the opinion that it is 4.10 material.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 2/2] x86/boot: rename send_chr to print_err

2017-10-13 Thread Doug Goldstein 
On 10/13/17 2:40 AM, Jan Beulich wrote:
 On 12.10.17 at 22:56,  wrote:
>> On 12/10/2017 21:50, Doug Goldstein wrote:
>>> From: David Esler 
>>>
>>> The send_chr function sends an entire C-string and not one character and
>>> doesn't necessarily just send it over the serial UART anymore so rename
>>> it to print_err so that its closer in name to what it does.
>>>
>>> Reviewed-by: Doug Goldstein 
>>> Signed-off-by: David Esler 
>>
>> Reviewed-by: Andrew Cooper 
>>
>> This should also be included in 4.10 IMO.
> 
> I'm not convinced - this is merely a cleanup style patch, the more
> that the label really serves dual purpose and hence the original
> name isn't all that wrong anyway.
> 
> Jan
> 

I purposefully broke it out so that it could be discussed. Prior to the
commit I referenced in patch 1 the function sent out a character. Now it
requires the supplied data to be a C-string (NULL terminated) so I would
hope that you could agree that at else the "chr" part of "send_chr" is
incorrect and should likely be "str" or "err".

But again this patch isn't really important its more to try to make the
label match the behavior of the code below the label and if its unwanted
then it can be dropped.

-- 
Doug Goldstein

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 2/2] x86/boot: rename send_chr to print_err

2017-10-13 Thread Jan Beulich 
>>> On 12.10.17 at 22:56,  wrote:
> On 12/10/2017 21:50, Doug Goldstein wrote:
>> From: David Esler 
>>
>> The send_chr function sends an entire C-string and not one character and
>> doesn't necessarily just send it over the serial UART anymore so rename
>> it to print_err so that its closer in name to what it does.
>>
>> Reviewed-by: Doug Goldstein 
>> Signed-off-by: David Esler 
> 
> Reviewed-by: Andrew Cooper 
> 
> This should also be included in 4.10 IMO.

I'm not convinced - this is merely a cleanup style patch, the more
that the label really serves dual purpose and hence the original
name isn't all that wrong anyway.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 2/2] x86/boot: rename send_chr to print_err

2017-10-12 Thread Daniel Kiper 
On Thu, Oct 12, 2017 at 09:56:01PM +0100, Andrew Cooper wrote:
> On 12/10/2017 21:50, Doug Goldstein wrote:
> > From: David Esler 
> >
> > The send_chr function sends an entire C-string and not one character and
> > doesn't necessarily just send it over the serial UART anymore so rename
> > it to print_err so that its closer in name to what it does.
> >
> > Reviewed-by: Doug Goldstein 
> > Signed-off-by: David Esler 
>
> Reviewed-by: Andrew Cooper 

Reviewed-by: Daniel Kiper 

Daniel

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 2/2] x86/boot: rename send_chr to print_err

2017-10-12 Thread Andrew Cooper 
On 12/10/2017 21:50, Doug Goldstein wrote:
> From: David Esler 
>
> The send_chr function sends an entire C-string and not one character and
> doesn't necessarily just send it over the serial UART anymore so rename
> it to print_err so that its closer in name to what it does.
>
> Reviewed-by: Doug Goldstein 
> Signed-off-by: David Esler 

Reviewed-by: Andrew Cooper 

This should also be included in 4.10 IMO.

> ---
>  xen/arch/x86/boot/head.S | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index f48bbbd2e5..22348b1bbe 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -161,7 +161,7 @@ not_multiboot:
>   */
>  add $sym_offs(.Lbad_ldr_nbs),%esi   # Error message
>  xor %edi,%edi   # No VGA text buffer
> -jmp .Lsend_chr
> +jmp .Lprint_err
>  .Lmb2_efi_ia_32:
>  /*
>   * Here we are on EFI IA-32 platform. Then reliable vga_text_buffer 
> zap is
> @@ -169,10 +169,10 @@ not_multiboot:
>   */
>  add $sym_offs(.Lbad_efi_msg),%esi   # Error message
>  xor %edi,%edi   # No VGA text buffer
> -jmp .Lsend_chr
> +jmp .Lprint_err
>  .Lget_vtb:
>  mov sym_esi(vga_text_buffer),%edi
> -.Lsend_chr:
> +.Lprint_err:
>  mov (%esi),%bl
>  inc %esi
>  test%bl,%bl# Terminate on '\0' sentinel
> @@ -185,11 +185,11 @@ not_multiboot:
>  mov %bl,%al
>  out %al,%dx# Send a character over the serial line
>  test%edi,%edi  # Is the VGA text buffer available?
> -jz  .Lsend_chr
> +jz  .Lprint_err
>  movsb  # Write a character to the VGA text buffer
>  mov $7,%al
>  stosb  # Write an attribute to the VGA text buffer
> -jmp .Lsend_chr
> +jmp .Lprint_err
>  .Lhalt: hlt
>  jmp .Lhalt
>  


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH 2/2] x86/boot: rename send_chr to print_err

2017-10-12 Thread Doug Goldstein 
From: David Esler 

The send_chr function sends an entire C-string and not one character and
doesn't necessarily just send it over the serial UART anymore so rename
it to print_err so that its closer in name to what it does.

Reviewed-by: Doug Goldstein 
Signed-off-by: David Esler 
---
 xen/arch/x86/boot/head.S | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index f48bbbd2e5..22348b1bbe 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -161,7 +161,7 @@ not_multiboot:
  */
 add $sym_offs(.Lbad_ldr_nbs),%esi   # Error message
 xor %edi,%edi   # No VGA text buffer
-jmp .Lsend_chr
+jmp .Lprint_err
 .Lmb2_efi_ia_32:
 /*
  * Here we are on EFI IA-32 platform. Then reliable vga_text_buffer 
zap is
@@ -169,10 +169,10 @@ not_multiboot:
  */
 add $sym_offs(.Lbad_efi_msg),%esi   # Error message
 xor %edi,%edi   # No VGA text buffer
-jmp .Lsend_chr
+jmp .Lprint_err
 .Lget_vtb:
 mov sym_esi(vga_text_buffer),%edi
-.Lsend_chr:
+.Lprint_err:
 mov (%esi),%bl
 inc %esi
 test%bl,%bl# Terminate on '\0' sentinel
@@ -185,11 +185,11 @@ not_multiboot:
 mov %bl,%al
 out %al,%dx# Send a character over the serial line
 test%edi,%edi  # Is the VGA text buffer available?
-jz  .Lsend_chr
+jz  .Lprint_err
 movsb  # Write a character to the VGA text buffer
 mov $7,%al
 stosb  # Write an attribute to the VGA text buffer
-jmp .Lsend_chr
+jmp .Lprint_err
 .Lhalt: hlt
 jmp .Lhalt
 
-- 
2.13.5


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel