Re: [XEN PATCH v2 3/3] libxl: Update the documentation of libxl_xen_console_read_line()

[Javi Merino <javi.merino@xxxxxxxxx>]

On Fri, Aug 23, 2024 at 06:31:50PM GMT, Andrew Cooper wrote:
> On 23/08/2024 6:05 pm, Javi Merino wrote:
> > diff --git a/tools/libs/light/libxl_console.c 
> > b/tools/libs/light/libxl_console.c
> > index f42f6a51ee6f..652897e4ef6d 100644
> > --- a/tools/libs/light/libxl_console.c
> > +++ b/tools/libs/light/libxl_console.c
> > @@ -789,17 +789,19 @@ libxl_xen_console_reader *
> >      return cr;
> >  }
> >  
> > -/* return values:                                          *line_r
> > - *   1          success, whole line obtained from buffer    non-0
> > - *   0          no more lines available right now           0
> > - *   negative   error code ERROR_*                          0
> > - * On success *line_r is updated to point to a nul-terminated
> > +/* Copy part of the console ring into a buffer
> > + *
> > + * Return values:
> > + *   1: Success, the buffer obtained from the console ring an
> > + *   0: No more lines available right now
> > + *   -ERROR_* on error
> > + *
> > + * On success, *line_r is updated to point to a nul-terminated
> 
> *buff.

Indeed.

> Also this really wants to say somewhere "despite the function name,
> there can be multiple lines in the returned buffer" or something to this
> effect.

Sure.  I had only written it in the commit message.  I have added it
to the documentation now.

> >   * string which is valid until the next call on the same console
> > - * reader.  The libxl caller may overwrite parts of the string
> > - * if it wishes. */
> > + * reader. */
> 
> While I don't want to derail this patch further, what happens if there
> happens to be an embedded \0 in Xen's console?  The hypercall is works
> by a count of chars, and AFACIT, in this function we're assuming that
> there's no \0 earlier in the string.

True.  This would have truncated the buffer before, and it still does
now.  libxl_xen_console_read_line() is the only one that knows the
size of the buffer and can't pass this information down to the caller,
so one way to fix this would be to scan the buffer for '\0' and
replace it with another character.

I'd rather do this in another patch by itself once this series is merged.


Cheers,
Javi