Re: [PATCH v2 4/4] drm/panic: Add a qr_code panic screen

[Jocelyn Falempe <jfalempe@redhat.com>]

On 09/07/2024 11:41, Miguel Ojeda wrote:
> Hi Jocelyn,
> 
> A quick docs-only review of the Rust side (some of these apply in
> several cases -- I just wanted to give an overview for you to
> consider).

Thanks, I'll fix all typo/grammar you mentioned.
> 
> On Tue, Jul 9, 2024 at 10:45 AM Jocelyn Falempe <jfalempe@redhat.com> wrote:
>>
>> +//! This is a simple qr encoder for DRM panic.
>> +//!
>> +//! Due to the Panic constraint, it doesn't allocate memory and does all
> 
> Perhaps clarify "Panic constraint" here?
> 
>> +//! the work on the stack or on the provided buffers. For
>> +//! simplification, it only supports Low error correction, and apply the
> 
> "applies"?
> 
>> +//! first mask (checkboard). It will draw the smallest QRcode that can
> 
> "QR code"? "QR-code"?
> 
> In other places "QR-code" is used -- it would be ideal to be
> consistent. (Although, isn't the common spelling "QR code"?)

Agreed, I will replace all with "QR code".

> 
>> +//! contain the string passed as parameter. To get the most compact
>> +//! QR-code, the start of the url is encoded as binary, and the
> 
> Probably "URL".

Yes, I will run s/url/URL in the comments.
> 
>> +//! compressed kmsg is encoded as numeric.
>> +//!
>> +//! The binary data must be a valid url parameter, so the easiest way is
>> +//! to use base64 encoding. But this waste 25% of data space, so the
> 
> "wastes"
> 
>> +//! whole stack trace won't fit in the QR-Code. So instead it encodes
>> +//! every 13bits of input into 4 decimal digits, and then use the
> 
> "uses"
> 
>> +//! efficient numeric encoding, that encode 3 decimal digits into
>> +//! 10bits. This makes 39bits of compressed data into 12 decimal digits,
>> +//! into 40bits in the QR-Code, so wasting only 2.5%. And numbers are
>> +//! valid url parameter, so the website can do the reverse, to get the
> 
> "And the numbers are valid URL parameters"?
> 
>> +//! Inspired by this 3 projects, all under MIT license:
> 
> "these"
> 
>> +// Generator polynomials for QR Code, only those that are needed for Low quality
> 
> If possible, please remember to use periods at the end for both
> comments and docs. It is very pedantic, but if possible we would like
> to try to be consistent across subsystems on how the documentation
> looks etc. If everything looks the same, it is also easy to
> remember/check how to do it for new files and so on.

Sure, I will check this again.
> 
>> +/// QRCode parameter for Low quality ECC:
>> +/// - Error Correction polynomial
>> +/// - Number of blocks in group 1
>> +/// - Number of blocks in group 2
>> +/// - Block size in group 1
>> +/// (Block size in group 2 is one more than group 1)
> 
> We typically leave a newline after a list.
> 
>> +    // Return the smallest QR Version than can hold these segments
>> +    fn from_segments(segments: &[&Segment<'_>]) -> Option<Version> {
> 
> Should be docs, even if private? i.e. `///`?
> 
> Also third person and period.
> 
>> +// padding bytes
>> +const PADDING: [u8; 2] = [236, 17];
> 
> `///`?
> 
>> +/// get the next 13 bits of data, starting at specified offset (in bits)
> 
> Please capitalize.
> 
>> +        // b is 20 at max (bit_off <= 7 and size <= 13)
> 
> Please use Markdown for comments too.
> 
>> +/// EncodedMsg will hold the data to be put in the QR-Code, with correct segment
>> +/// encoding, padding, and Error Code Correction.
> 
> Missing newline? In addition, for the title (i.e. first paragraph), we
> try to keep it short/simple, e.g. you could perhaps say something
> like:
> 
>      /// Data to be put in the QR code (with correct segment encoding,
> padding, and error code correction).
> 
>> +/// QrImage
>> +///
>> +/// A QR-Code image, encoded as a linear binary framebuffer.
> 
> Please remove the title -- the second paragraph should be the title.
> 
>> +/// Max width is 177 for V40 QR code, so u8 is enough for coordinate.
> 
> `u8`
> 
>> +/// drm_panic_qr_generate()
> 
> You can remove this title.
> 
>> +/// C entry point for the rust QR Code generator.
>> +///
>> +/// Write the QR code image in the data buffer, and return the qrcode size, or 0
>> +/// if the data doesn't fit in a QR code.
>> +///
>> +/// * `url` The base url of the QR code. It will be encoded as Binary segment.
> 
> Typically we would write a colon. after the key, e.g.
> 
>      /// * `url`: the base URL of the QR code.
> 
>> +/// # Safety
>> +///
>> +/// * `url` must be null or point at a nul-terminated string.
>> +/// * `data` must be valid for reading and writing for `data_size` bytes.
>> +/// * `data_len` must be less than `data_size`.
>> +/// * `tmp` must be valid for reading and writing for `tmp_size` bytes.
> 
> It would be nice to mention for which duration these need to hold,
> e.g. the call or something else.

Yes, it's until the function returns, I will add this precision.

> 
>> +        // Safety: url must be a valid pointer to a nul-terminated string.
> 
> Please use the `// SAFETY: ` prefix instead, since it is how we tag
> these (i.e. differently from from the `# Safety` section).
> 
>> +/// * `version` QR code version, between 1-40.
> 
> If something like this happens to be used in several places, you may
> want to consider using transparent newtypes for them. This would allow
> you to avoid having to document each use point and it would enrich the
> signatures.
> 

I used to list all QR versions in an enum, but I find it a bit too much 
boilerplate to ensure the version is between 1 and 40.
By transparent newtypes, you mean adding "#[repr(transparent)]" to a 
struct ?
I don't plan to add more "version" usage, so probably not worth it.

> Thanks!
> 
> Cheers,
> Miguel
> 

Best regards,

-- 

Jocelyn