| Message ID | 20171012205007.13001-2-cardoe@cardoe.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/10/2017 21:50, Doug Goldstein wrote: > From: David Esler <drumandstrum@gmail.com> > > 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 <cardoe@cardoe.com> > Signed-off-by: David Esler <drumandstrum@gmail.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> 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 >
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 <drumandstrum@gmail.com> > > > > 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 <cardoe@cardoe.com> > > Signed-off-by: David Esler <drumandstrum@gmail.com> > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> Daniel
>>> On 12.10.17 at 22:56, <andrew.cooper3@citrix.com> wrote: > On 12/10/2017 21:50, Doug Goldstein wrote: >> From: David Esler <drumandstrum@gmail.com> >> >> 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 <cardoe@cardoe.com> >> Signed-off-by: David Esler <drumandstrum@gmail.com> > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > > 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
On 10/13/17 2:40 AM, Jan Beulich wrote: >>>> On 12.10.17 at 22:56, <andrew.cooper3@citrix.com> wrote: >> On 12/10/2017 21:50, Doug Goldstein wrote: >>> From: David Esler <drumandstrum@gmail.com> >>> >>> 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 <cardoe@cardoe.com> >>> Signed-off-by: David Esler <drumandstrum@gmail.com> >> >> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> >> >> 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.
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, <andrew.cooper3@citrix.com> wrote: >>> On 12/10/2017 21:50, Doug Goldstein wrote: >>>> From: David Esler <drumandstrum@gmail.com> >>>> >>>> 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 <cardoe@cardoe.com> >>>> Signed-off-by: David Esler <drumandstrum@gmail.com> >>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> >>> 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
>>> On 13.10.17 at 18:07, <cardoe@cardoe.com> wrote: > On 10/13/17 2:40 AM, Jan Beulich wrote: >>>>> On 12.10.17 at 22:56, <andrew.cooper3@citrix.com> wrote: >>> On 12/10/2017 21:50, Doug Goldstein wrote: >>>> From: David Esler <drumandstrum@gmail.com> >>>> >>>> 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 <cardoe@cardoe.com> >>>> Signed-off-by: David Esler <drumandstrum@gmail.com> >>> >>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> >>> 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
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