Toomas, thanks for putting this all together. I know that I have a lot of comments, but I think that the vast majority of this is good and I know represents a lot of hard work. Thanks!

usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)

Show all issues

Maybe add a comment that this is initialized to the BIOS based version and that in the gfx_framework_init() function will change it to use UEFI methods if appropriate?

usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)

Show all issues

Probably should use ARRAY_SIZE(solaris_color_to_pc_color), in case we ever change the types here.

1. 

   Toomas Soome Dec. 12, 2018, 10:15 a.m.

   in loader, it is nitems()

usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)

Show all issues

Does some other part of the stack guarantee that index will not go beyond the array size?

1. 

   Toomas Soome Dec. 12, 2018, 10:15 a.m.

   indirectly; we start up with default foreground and background colors and only way to change those is via esc sequences sent to tem, and there we do have fixed list of possible values. If we allow to set color directly to lit the pixel or we do implement interface to allow custom RGB values to be loaded, then this should be reviewed.

usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)

Show all issues

Do we not care about conversion errors from the strtol or cases where the string is not a valid string like '34asdf'?

1. 

   Toomas Soome Dec. 12, 2018, 10:15 a.m.

   it is nice to care, so I did update..

2. 

   Robert Mustacchi Dec. 14, 2018, 9:19 p.m.

   Thanks.

usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)

Show all issues

Should we worry about the case where the string name is neither 'tem.inverse' or 'tem.inverse-screen'?

1. 

   Toomas Soome Dec. 11, 2018, 7:02 a.m.

   no, this callback is set in gfx_framework_init() and bond to those two variables, we will only be called when user is about to set one of those.

usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)

Show all issues

Is the framebuffer endianness well defined to be little endian or does it match the host endianness? If the first case, we probably need to adjust the cases here to do conversion.

1. 

   Toomas Soome Dec. 11, 2018, 6:58 a.m.

   Yes, I would wait for actual case here.

2. 

   Joshua Clulow Dec. 12, 2018, 9:55 p.m.

   It seems fine to avoid writing the endian conversion code, but in it's place we should probably have a compile-time check? e.g., https://github.com/joyent/illumos-joyent/blob/8a15db18cc817a39246b143e7ccd35a0898ed743/usr/src/uts/common/sys/scsi/adapters/smrt/smrt.h#L38-L44

3. 

   Robert Mustacchi Dec. 14, 2018, 9:19 p.m.

   I'm fine if we want to punt on it, let's just make it explicitly fail at compile time.

usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)

Show all issues

Should there by a default case here which results in an error?

1. 

   Toomas Soome Dec. 12, 2018, 10:15 a.m.

   tem actually does not do anything about the error...

usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)

Show all issues

These should probably be unsigned to match the places that they come from. With pitch we're truncating the uint32_t to an int.

1. 

   Toomas Soome Dec. 11, 2018, 12:25 p.m.

   Yes, I have bunch of those to get unsigned, the extra warnings will trigger otherwise...

usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)

Show all issues

I'm not sure if the calculation of blend guarantees this property or not, but if somehow blend = 0xffff, then we'll have h = 0xff, l = 0xff, which will cause h to be set to 0x100, which we will truncate to zero at the return. Is that the right behavior? Should values greater than a uint8_t in h be truncated versus round up to UINT8_MAX?

1. 

   Toomas Soome Dec. 11, 2018, 11:39 a.m.

   We can not get blend to 0xffff. The maximum we can get is fg = 0xff, bg = 0xff, and alpha = 0xfe (0xff is corner case). in such setup, we will have blend 0xfe01 and that will result with h = 0xff.

usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)

Show all issues

Why do we only do alpha blending in the 4 bits per pixel case? Maybe worth a comment explaining that. It's not clear to me why this case is special and the rest default into a memcpy.

1. 

   Toomas Soome Dec. 11, 2018, 11:10 a.m.

   ay, this one is confusing because the bpp here is bytes per pixel. comment added.

usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)

Show all issues

s/baack/back

usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)

Show all issues

I guess there's nothing we can really do if this or the next call fail, is there?

1. 

   Toomas Soome Dec. 11, 2018, 10:14 a.m.

   Yep. There are two error cases:
   
   BltOperation is not valid. -- we should not fail this ever.
   
   The device had an error and could not complete the request. -- this one might happen, but if so, we can not really do anything about it at this point.

usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)

Show all issues

Can we add a comment somewhere in the file about when we know we have to deal with the TPL? As if I had to modify this or add something new, I'd never know.

usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)

Show all issues

Minor note: inconsistent function declaration style, missing return after type.

usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)

Show all issues

This gets the case where we have no fb and we're outputing only to serial or similar?

1. 

   Toomas Soome Dec. 11, 2018, 10:14 a.m.

   if we have BIOS loader and the console is set to text mode. Note the "direct" drawing functions like setpixel() are still flawed for UEFI - in case we actually do not have physical frame buffer and can only use Blt() to draw. But this flaw should be dealt later (especially because I have nowhere to test, and our kernel FB console would be dead too).

usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)

Show all issues

Do we need to free fp->vf_map[n] before returning here?

1. 

   Toomas Soome Dec. 11, 2018, 10:14 a.m.

   no, but I'll add it anyhow for consistency. The load_font() will clean up anyhow.

usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)

Show all issues

Can we expand this comment to say why we should just replace the name?

usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)

Show all issues

I was going through other callers a bit and was confused when the name argument was never freed. I think that the caller here should do the strdup of name. That might make it clearer to others what's going on.

usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)

Show all issues

Is it OK that insert_font() will take ownership of np->n_name?

1. 

   Toomas Soome Dec. 12, 2018, 10:15 a.m.

   we better allocate own instance..

usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)

Show all issues

If we don't change anything about insert_font(), then this should have a comment that when insert_font is successful, it takes ownership over name, so it's OK that we never free it after this point. That confused me for a bit.

usr/src/boot/sys/boot/common/help.common (Diff revision 1)

Show all issues

'the default resolution will be set to 800x600.'

usr/src/boot/sys/boot/common/module.c (Diff revision 1)

Show all issues

Does this need to live in a shared header?

Unrelated, but I like the strategy. It seems to make sense. This shows up as a module in the multiboot2 sense right? We won't create a boot time module from it, will we?

1. 

   Toomas Soome Dec. 12, 2018, 8:57 a.m.

   Yes, the idea is that:
   
   1. we have compiled in (limited) font - 8x16, to be able to have basic screen output on FB. 8x16 was selected because it can be shared with VGA text mode too. The alternative implementation could be to keep text mode/SimpleTextOutputProtocol till we have font loaded.
   
   2. if we have font loaded (manual or autoload), it is full font and used by loader, and if FB is supported by the kernel, we will pass this font down to kernel as multiboot module (type=console-font). This module has small header with pointers to data and it is in memory representation of the loaded font.
   
   3. kernel also will have small built in font for case we get framebuffer, or to load vga text mode font, but if the console-module font was passed, it will be used instead - in early boot by using the module memory directly, and later the memory will be allocated and the module will be released.
   
   4. At some point in future, we can implement font loading to kernel - the base mechanism to switch the font is already present, but the tooling and the data transfer to kernel is to be implemented. However, I would assume that for most cases the boot loader mechanism will be preferred anyhow.

2. 

   Robert Mustacchi Dec. 14, 2018, 9:19 p.m.

   OK, thanks for the additional details here. This makes sense. I like it.

usr/src/boot/sys/boot/common/multiboot2.c (Diff revision 1)

Show all issues

Is it possible to add a #define to i386/libi386/vbe.h
to cover the different mode arguments to bios_set_text_mode?

usr/src/boot/sys/boot/common/multiboot2.c (Diff revision 1)

Show all issues

It'd be nice to have a constant for the value 0x100 to make it a little less magical. It's not clear to me exactly why it's 0x100, but I guess that's the first VBE mode value from looking at the VBE 3.0 spec.

usr/src/boot/sys/boot/common/multiboot2.c (Diff revision 1)

Show all issues

Maybe comment this EFI endif?

usr/src/boot/sys/boot/common/multiboot2.c (Diff revision 1)

Show all issues

Is EFI Guaranteed to never have the indexed type framebuffer?

Also, don't we need to check the result of the mb_alloc()?

1. 

   Toomas Soome Dec. 11, 2018, 7:22 a.m.

   never is such an long time, but I think this is quite unlikely - at least for now we only do get 32-bit depths. mb_malloc is explained below.

usr/src/boot/sys/boot/common/multiboot2.c (Diff revision 1)

Show all issues

Don't we need to check the result of the mb_alloc?

1. 

   Toomas Soome Dec. 11, 2018, 7:17 a.m.

   no, we count the size for mb info, then allocate it, and mb_malloc() will get chunks from this area.

usr/src/boot/sys/boot/common/tem.c (Diff revision 1)

Show all issues

How different is this from the kernel version? I assume there's no good way to share the two and it's worth just having a separate copy? I wasn't sure if this was more of a straight up copy with the kernel module bits removed or not, so I haven't looked at this file in great detail.

1. 

   Toomas Soome Dec. 11, 2018, 8:58 a.m.

   As noted in opening comment - I am afraid this one will be quite impossible to share; with full rewrite, if we would separate out the terminal emulator state automata, we could get the common code. But I am not sure if this is worth the hassle.

usr/src/boot/sys/boot/efi/include/eficon.h (Diff revision 1)

Show all issues

I noticed this is missing a number of scan codes from the spec. Should we add them or wait for a later date?

1. 

   Toomas Soome Dec. 12, 2018, 11:44 a.m.

   Those are all there but seplit bad. rearranged.

usr/src/boot/sys/boot/efi/libefi/efi_console.c (Diff revision 1)

Show all issues

I gues we don't care if this fails since there isn't much we can do, right?

1. 

   Toomas Soome Dec. 11, 2018, 9:01 a.m.

   Yes. in worst case, if the menu (where we really need the terminal emulation) is distorted, we can always just drop to cli (ok) prompt and boot manually. And in any case, for 99.999% cases the boot screen is visible for countdown time:D

usr/src/boot/sys/boot/efi/libefi/efi_console.c (Diff revision 1)

Show all issues

I guess we don't care if this fails? Or is it more of the case that there's nothing we can do? There are a bunch of other cases like this in this file, but I'm not going to call them each out as I imagine it'll be the same thing.

usr/src/boot/sys/boot/efi/libefi/efi_console.c (Diff revision 1)

Show all issues

Why does this case use tem to hide the cursor, but the other case to show it doesn't not when we're in the !plat_stdout_is_framebuffer()?

1. 

   Toomas Soome Dec. 12, 2018, 12:28 p.m.

   the function name only is hinting it was created for tem; since the kernel tem does require its presence, I did kept the same for loader. In kernel it is only meaningful for sparc, to switch the firmware cursor off.
   but for feature patity, I did factor out those bits now.

usr/src/boot/sys/boot/efi/libefi/efi_console.c (Diff revision 1)

Show all issues

Is the only used at startup why this doesn't have a framebuffer case like the others?

1. 

   Toomas Soome Dec. 12, 2018, 12:28 p.m.

   Yes, this is also originating from sparc prom, when tem is starting up, we fetch the prom cursor position to continue where from we were left to. So we only get the cursor position in terms of terminal coordinates and the tem will do the math. Note that here it does not really matter if we can preserve the cursor position because switching the font will clear the whole screen anyhow.

usr/src/boot/sys/boot/efi/libefi/efi_console.c (Diff revision 1)

Show all issues

Can we say that it doesn't have enough memory while probing the console so we have a sense of where in the code base we got to?

usr/src/boot/sys/boot/efi/libefi/efi_console.c (Diff revision 1)

Show all issues

snprintf?

usr/src/boot/sys/boot/efi/libefi/efi_console.c (Diff revision 1)

Show all issues

Is it OK to continue if it failed? Does this happen when mirroring or another case? Probably worth an extra comment.

usr/src/boot/sys/boot/efi/libefi/efi_console.c (Diff revision 1)

Show all issues

Clean up whitespace?

usr/src/boot/sys/boot/efi/loader/framebuffer.c (Diff revision 1)

Show all issues

Space between function and open paren?

usr/src/boot/sys/boot/efi/loader/framebuffer.c (Diff revision 1)

Show all issues

Is there a reason we're not checking for x being the character after the strtoul finished? Or are there fonts that are of a different form from %ux%u?

usr/src/boot/sys/boot/efi/loader/framebuffer.c (Diff revision 1)

Show all issues

Same thing about strchr versus strtoul end pointer.

usr/src/boot/sys/boot/efi/loader/framebuffer.c (Diff revision 1)

Show all issues

efifb_color_depth returns an unsigned integer, but we're assigning it to a signed integer. Maybe i and d should be unsigned?

usr/src/boot/sys/boot/forth/frames.4th (Diff revision 1)

Show all issues

Can you briefly explain why all the starting constants changed here?

1. 

   Toomas Soome Dec. 12, 2018, 1:23 p.m.

   To avoid translation pain. the UEFI text console is unicode, our console fonts are unicode, so we can just use xemit to emit unicode box chars and let the low level console code to sort the final output - bitmaps are drawn, UEFI simple text output will pring unicode and vga text mode will translate unicode to its own box chars.

usr/src/boot/sys/boot/i386/libi386/spinconsole.c (Diff revision 1)

Show all issues

What if we don't find a text console here? Is that possible? If we don't, won't we write over the end of the array?

1. 

   Toomas Soome Dec. 12, 2018, 5:04 p.m.

   The consoles (and block devices and file systems) are built on static arrays and "text" is always present, so yea, we can take it granted. Nevertheless it is good idea to have some guard...

usr/src/boot/sys/boot/i386/libi386/vbe.h (Diff revision 1)

Show all issues

Maybe we can use this in the place where I pointed out the confusion with the >= 0x100 in multiboot.

usr/src/boot/sys/boot/i386/libi386/vbe.c (Diff revision 1)

Show all issues

This code looks rather familiar at this point. Maybe we can commonize it?

usr/src/boot/sys/boot/i386/libi386/vbe.c (Diff revision 1)

Show all issues

Is there a reason we're converting unsigned values to signed ones?

usr/src/boot/sys/boot/i386/libi386/vbe.c (Diff revision 1)

Show all issues

Probably should use snprintf for these command error buffer prints so we don't clobber global memory.

usr/src/boot/sys/boot/i386/libi386/vgasubr.c (Diff revision 1)

Show all issues

This looks fairly similar to the existing vgasubr.c. Is it possible to share it in usr/src/common and ifdef STANDALONE versus KERNEL? Or is it impossible because of the use of the struct vgaregmap there? It just seems unfortunate to have almost two identical copies.

usr/src/boot/sys/sys/ascii.h (Diff revision 1)

Show all issues

See other notes on shared headers on later files.

usr/src/boot/sys/sys/consplat.h (Diff revision 1)

Show all issues

See other notes on shared headers on later files.

usr/src/boot/sys/sys/kd.h (Diff revision 1)

Show all issues

Is there no way to share this header?

usr/src/boot/sys/sys/list.h (Diff revision 1)

Show all issues

Is there no way to avoid duplicating this header?

usr/src/boot/sys/sys/list_impl.h (Diff revision 1)

Show all issues

Is there no way to avoid duplicating this header?

usr/src/boot/sys/sys/tem.h (Diff revision 1)

Show all issues

See later notes on header files.

1. 

   Toomas Soome Dec. 12, 2018, 8:19 p.m.

   yea, I'd leave tem headers duplicated for now. To build the unicode support, the kernel tem will need similar changes and once done, we can see how we can deal with those.

usr/src/boot/sys/sys/tem_impl.h (Diff revision 1)

Show all issues

See other header files for notes on sharing (sorry these were reviewed in reverse order on this page). I know this one is a bit more different than others.

usr/src/boot/sys/sys/vgareg.h (Diff revision 1)

Show all issues

Aside from these definitions, this looks the same as the existing kernel header. See other comments about these, but maybe we can share a definition in usr/src/common/ so that way we can have the two stay roughly in sync rather than duplicating them?

usr/src/boot/sys/sys/vgasubr.h (Diff revision 1)

Show all issues

Is there a reason we're using u_int rather than uint_t? I guess because of the difference in arguments there's no good way to share this with the pre-existing copy? Maybe we could do something like typedef the first argument and ifdef KERNEL it's the struct otherwise ifdef STANDALONE it's the uint_t?

usr/src/boot/sys/sys/visual_io.h (Diff revision 1)

Show all issues

It looks like this header is the exact same as uts/common/sys/visual_io.h with the exception of adding the following:

\* struct visual_ops
\* 16-bit data to the color array
\* VIS_CONSCLEAR

Is there a reason we can't actually combine this back with the one in uts and share the header between the two rather than just duplicating it in its entirety?

If we're worried about all the intersections between the kernel due to putting it on the loader include file search path, maybe we can share them in common/?

1. 

   Toomas Soome Dec. 12, 2018, 8:19 p.m.

   this is a bit fishy, but well...

usr/src/common/ficl/loader.c (Diff revision 1)

Show all issues

Just to make sure in this case to ficl, ret = -1 is success?

1. 

   Toomas Soome Dec. 9, 2018, 9:50 a.m.

   Yes, in forth, the -1 is traditional notation for true or success.

2. 

   Gergő Mihály Doma Dec. 10, 2018, 12:40 p.m.

   
   common/ficl/ficl.h#590 already contains definitions for FICL_TRUE and FICL_FALSE. Maybe we could use these macro constants or create the next two and use those to lessen the code ambiguity (in general, not just here).
   
   #define FICL_SUCCESS FICL_TRUE
   #define FICL_FAILURE FICL_FALSE
   
   
   Type of ret variable should be ficlInteger, not int.
   

3. 

   Toomas Soome Dec. 10, 2018, 1:39 p.m.

   good point, fixed.

usr/src/common/ficl/loader.c (Diff revision 1)

Show all issues

Mind putting the closing #endif comment ala:

#endif /* STANDALONE */

usr/src/common/font/font.c (Diff revision 1)

Show all issues

Should we remove the kernel copy of this file for the time being and use its copy?

1. 

   Toomas Soome Dec. 9, 2018, 9:50 a.m.

   Yes, but later, once we have the kernel updates in place. We also will drop the current fonts built into the kernel etc and to avoid "prermaturely" touching the kernel, I did create this patch dedicated only for the loader part.

usr/src/common/font/font.c (Diff revision 1)

Show all issues

Worth throwing in the enum pc_colors like the other users used to do?

usr/src/common/font/font.c (Diff revision 1)

Show all issues

Shouldn't these be unsigned values, since the font_map works in unsigned values and the arguments are unsigned?

1. 

   Toomas Soome Dec. 12, 2018, 10:04 p.m.

   I think it should be safe, as we first do perform the linear search so all values should get well above 0:)

usr/src/common/font/font.c (Diff revision 1)

Show all issues

Do we care about the special case where we don't find something and get to dst == 0. Is the first entry in vf_bytes below going to be correct?

1. 

   Toomas Soome Dec. 12, 2018, 10:04 p.m.

   Yes. The code is built to process font data where we can have both normal and bold glyphs, with fallback to normal. For example, Gallant does only provide normal (its actually semibold). But also it may be that some glyphs are missing from bold set. If dst is still 0, we have special corner case where font very first glyph is one used to denote the missing symbol (? or empty rectangle or like).

usr/src/common/font/font.c (Diff revision 1)

Show all issues

Why is this the only one that checks the TEM_CHAR_ATTR andd the others do not. If that's how it should be, can you add a comment explaining why please?

1. 

   Toomas Soome Dec. 9, 2018, 10:30 a.m.

   my bug.

usr/src/common/pnglite/pnglite.h (Diff revision 1)

Show all issues

I think we need ot make sure a THIRDPARTYLICENSE and THIRDPARTYLICENSE.descrip are added to cover this and that they are in the corresponding manifest.

usr/src/common/pnglite/pnglite.c (Diff revision 1)

Show all issues

I have a bunch of comments on this code. I'm not sure how much modification we want to do to the third party code, but I wanted to mention it. Though I imagine that we may have had to modify it for this.

usr/src/common/pnglite/pnglite.c (Diff revision 1)

Show all issues

lseek and read return ssize_t values, but this is returning a size_t. This means that we're definitely transforming that in a way that would make it look like we read a lot of data on failure.

usr/src/common/pnglite/pnglite.c (Diff revision 1)

Show all issues

The (off_t)(size \* numel) is taking two unsigned values, multiplying them, and casting them to a signed value. We should probably check for overflow.

usr/src/common/pnglite/pnglite.c (Diff revision 1)

Show all issues

Minor style nit, this function is different from the prior ones with respect to cstyle in the return after the return type.

usr/src/common/pnglite/pnglite.c (Diff revision 1)

Show all issues

Minor style nit, this function is different from the prior ones with respect to cstyle in the return after the return type.

usr/src/common/pnglite/pnglite.c (Diff revision 1)

Show all issues

Shouldn't this only be called if w're in the result == PNG_NO_ERROR case. Perhaps at +188.

usr/src/common/pnglite/pnglite.c (Diff revision 1)

Show all issues

Do we care about someone being able to overflow here?

usr/src/common/pnglite/pnglite.c (Diff revision 1)

Show all issues

If we hit this case, then we will never free png->zs. I'm not sure where the right place to try and free this up is, but certainly looking at the way callers work today, this'll be leaked. I could see us doing one of two things. One is to always free it in png_end_inflate when we have a zlib error. The other is to try and keep this around and free it on close. However, that has its own risks because we would make sure that we freed it before another call to png_start_inflate.

Maybe we should do both? Try and free it in png_end_inflate, but also try and catch someone not freeing it in png_close() and someone doing the same in png_start_inflate()?

What do you think makes sense?

1. 

   Toomas Soome Dec. 10, 2018, 8:39 a.m.

   The normal approach should be that we do not end up with partial setup. So we will always free in png_end_inflate() and clean up in case of error in png_init_inflate()

usr/src/common/pnglite/pnglite.c (Diff revision 1)

Show all issues

I assume we don't want to use realloc?

usr/src/common/pnglite/pnglite.c (Diff revision 1)

Show all issues

Are there endian concerns here? My read is that this data is in Big Endian, but we're just taking an array of bytes and casting it to a uint32_t, which I think means we'll read that in an endian aware way.

usr/src/common/pnglite/pnglite.c (Diff revision 1)

Show all issues

All of the filter length / stride values could probably be unsigned. This applies to the others as well.

usr/src/common/pnglite/pnglite.c (Diff revision 1)

Show all issues

Why are we casting this to a char versus a uint8_t? Unfortunately a default 'char' can either be signed or unsigned based on the processor / platform. ARM for example, historically has had the opposite of x86.

1. 

   Toomas Soome Dec. 9, 2018, 7:51 p.m.

   I would believe, they were intending to achieve (in[i] + sum/2) & 0xff here.

usr/src/common/pnglite/pnglite.c (Diff revision 1)

Show all issues

This converts an unsigned data to a signed data. It won't overflow, but seems odd to change the sign.

usr/src/man/man5/loader.5 (Diff revision 1)

Show all issues

Are COLUMNS and LINES no longer honored? Or is it only no longer honored in the context of using the VGA frame buffer? Just trying to understand why this changed.

1. 

   Toomas Soome Dec. 9, 2018, 9:50 a.m.

   I did decide to go with screen-* because we have those properties in our kernel (wscons.c): screen-#cols, screen-#rows, screen-width, screen-height. We build up the console properties in loader and pass the values down to kernel via environment module and the kernel will inherit and use the values (down to cursor position). This will allow us to get consistent console and will save us from translating loader properties to kernel properties in kernel fakebop.c.

Re-defined common macros.

usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 2)

Is it worth to add an ABS() macro into <sys/param.h>?

(Not for this issue.)

usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 2)

Show all issues

This macro already exist in <sys/param.h>, but that's name is capitalized.
And it's not used, so delete it.

usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 2)

abs() macro used here + in the next line.

usr/src/common/pnglite/pnglite.c (Diff revision 2)

#ifdef _STANDALONE
#include <sys/param.h>

#ifndef ABS
/* should be in <sys/param.h> */
#define ABS(x)      ((x) < 0 ? -(x) : (x))
#endif

#else /* !_STANDALONE */
#include <sys/sysmacros.h>
#endif

(The preferred macro name should be the capitalized name.)

1. 

   Toomas Soome Dec. 10, 2018, 12:52 p.m.

   abs() is tricky because it is function in userland (man abs).

usr/src/common/pnglite/pnglite.c (Diff revision 2)

abs() used here + in the next 2 lines.

usr/src/common/ficl/loader.c (Diff revision 6)

Show all issues

names should be an unsigned value.
...
ficlUnsigned names;
...
names = ficlStackPopUnsigned(ficlVmGetDataStack(pVM));

Duplicated macro.

usr/src/boot/sys/boot/uboot/common/main.c (Diff revision 19)

<sys/param.h> already contains this nitems macro.

1. 

   Toomas Soome Dec. 14, 2018, 7:05 a.m.

   yes, but not related to this work:)

usr/src/boot/sys/boot/common/gfx_fb.c (Diff revisions 1 - 19)

Show all issues

Should we be using CMD_OK or should we actually be using CMD_WARN/CMD_ERROR in cases where the user input something incorrect?

1. 

   Toomas Soome Dec. 15, 2018, 7:03 a.m.

   There is a problem with those codes - when we get the error while interpreting startup scripts, the interpeter will stop. So we have one hand with lets set an error and other hand with should we break the boot process. I guess with colors we should rather keep boot going:)

2. 

   Robert Mustacchi Dec. 17, 2018, 11:02 p.m.

   OK, this makes sense. Yeah, we'd rather try to boot, I think. Seems like in the future we should separate the fatalness of the two operations somehow, but that's another day's problem.

usr/src/boot/sys/boot/common/gfx_fb.c (Diff revisions 1 - 19)

Show all issues

I think this is always safe, but I'll be honest, seeing the cast was a bit weird at first glance.

In general, I think this is good, though if you don't mind, I'd still like the explicit build failure if we're building big endian that jclulow described.

usr/src/boot/sys/boot/common/tem.c (Diff revisions 20 - 21)

Show all issues

Unchecked return value? Since this returns void, I guess we should panic on failure?

1. 

   Toomas Soome Dec. 22, 2018, 10:02 a.m.

   Yep. I can not get any other good option.

usr/src/boot/sys/boot/common/tem.c (Diff revisions 20 - 21)

Show all issues

Unchecked return value? Since this returns void, I guess we should panic on failure?

1. 

   Toomas Soome Dec. 21, 2018, 6:21 p.m.

   tvs_screen_buf is simple - if we fail to allocate, we will not use it (the code has checks in place). We fall back on full line copy in copy area.

2. 

   Robert Mustacchi Dec. 21, 2018, 8:36 p.m.

   OK, can I recommend a comment there that mentions that?

3. 

   Toomas Soome Dec. 22, 2018, 10:02 a.m.

   yes, good point.

usr/src/boot/sys/boot/common/tem.c (Diff revisions 20 - 21)

Show all issues

Should width and cols be unsigned here since we're using them as indexes into an array? Though that change would require redoing the way the while loop works due to the comparison / wraparound.

1. 

   Toomas Soome Dec. 22, 2018, 10:02 a.m.

   yea, I do not think using unsigned value will serve any good purpose there except we will complicate things.

usr/src/boot/sys/boot/common/tem.c (Diff revisions 20 - 21)

Show all issues

It might be worth a comment explaining why these two cases (this one and the one a few lines later) are here.

usr/src/boot/sys/boot/common/tem.c (Diff revisions 20 - 21)

Show all issues

Should we maybe use the constants for these?

1. 

   Toomas Soome Dec. 21, 2018, 6:21 p.m.

   ya, well, and those actually only matter for debug purposes (to paint the image area).

2. 

   Toomas Soome Dec. 22, 2018, 10:02 a.m.

   removed them.

usr/src/boot/sys/boot/common/tem.c (Diff revisions 20 - 21)

Show all issues

Why is this bit in an ifdef DEBUG?

1. 

   Toomas Soome Dec. 21, 2018, 6:21 p.m.

   Literally for debug - so the developer can see the area:)

2. 

   Toomas Soome Dec. 22, 2018, 10:02 a.m.

   removed it.

usr/src/boot/sys/boot/common/tem.c (Diff revisions 20 - 21)

Show all issues

Maybe unsigned count or change the loop to be > 0?

Ship It!

Ship It!