| Message ID | d868579d9ea98d8072630f5b85f7f7250ece393d.1724430173.git.javi.merino@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libxl: Fixes for libxl_xenconsole_readline() | expand |
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. 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. > * 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. ~Andrew
On Fri, Aug 23, 2024 at 06:05:05PM +0100, Javi Merino wrote: > Despite its name, libxl_xen_console_read_line() does not read a line, > it fills the buffer with as many characters as fit. Update the > documentation to reflect the real behaviour of the function. Rename > line_r to avoid confusion since it is a pointer to an array of > characters. > > Signed-off-by: Javi Merino <javi.merino@cloud.com> > --- > tools/libs/light/libxl_console.c | 22 ++++++++++++---------- > 1 file changed, 12 insertions(+), 10 deletions(-) > > 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 > * 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. */ > int libxl_xen_console_read_line(libxl_ctx *ctx, > libxl_xen_console_reader *cr, > - char **line_r) > + char **buff) You may want to update "tools/include/libxl.h" as well. I don't know why this comments is here instead of in the public header. At least there's some documentation I guess. Cheers,
On Fri Aug 23, 2024 at 6:05 PM BST, Javi Merino wrote: > Despite its name, libxl_xen_console_read_line() does not read a line, > it fills the buffer with as many characters as fit. Update the > documentation to reflect the real behaviour of the function. Rename > line_r to avoid confusion since it is a pointer to an array of > characters. > > Signed-off-by: Javi Merino <javi.merino@cloud.com> > --- > tools/libs/light/libxl_console.c | 22 ++++++++++++---------- > 1 file changed, 12 insertions(+), 10 deletions(-) > > 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 Seems like this line in the comment is incomplete? > + * 0: No more lines available right now > + * -ERROR_* on error > + * > + * On success, *line_r is updated to point to a nul-terminated > * 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. */ Cheers, Alejandro
On Mon, Sep 02, 2024 at 11:14:11AM GMT, Alejandro Vallejo wrote: > On Fri Aug 23, 2024 at 6:05 PM BST, Javi Merino wrote: > > Despite its name, libxl_xen_console_read_line() does not read a line, > > it fills the buffer with as many characters as fit. Update the > > documentation to reflect the real behaviour of the function. Rename > > line_r to avoid confusion since it is a pointer to an array of > > characters. > > > > Signed-off-by: Javi Merino <javi.merino@cloud.com> > > --- > > tools/libs/light/libxl_console.c | 22 ++++++++++++---------- > > 1 file changed, 12 insertions(+), 10 deletions(-) > > > > 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 > > Seems like this line in the comment is incomplete? Indeed. Fixed to say: 1: Success, *buff points to the string Thanks, Javi
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
On Tue, Aug 27, 2024 at 03:26:09PM GMT, Anthony PERARD wrote: > On Fri, Aug 23, 2024 at 06:05:05PM +0100, Javi Merino wrote: > > Despite its name, libxl_xen_console_read_line() does not read a line, > > it fills the buffer with as many characters as fit. Update the > > documentation to reflect the real behaviour of the function. Rename > > line_r to avoid confusion since it is a pointer to an array of > > characters. > > > > Signed-off-by: Javi Merino <javi.merino@cloud.com> > > --- > > tools/libs/light/libxl_console.c | 22 ++++++++++++---------- > > 1 file changed, 12 insertions(+), 10 deletions(-) > > > > 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 > > * 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. */ > > int libxl_xen_console_read_line(libxl_ctx *ctx, > > libxl_xen_console_reader *cr, > > - char **line_r) > > + char **buff) > > You may want to update "tools/include/libxl.h" as well. Sure, I will change this in "tools/include/libxl.h". > I don't know why > this comments is here instead of in the public header. At least there's > some documentation I guess. Cheers, Javi
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 * 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. */ int libxl_xen_console_read_line(libxl_ctx *ctx, libxl_xen_console_reader *cr, - char **line_r) + char **buff) { int ret; /* number of chars to copy into the buffer. xc_readconsolering() @@ -818,10 +820,10 @@ int libxl_xen_console_read_line(libxl_ctx *ctx, if (!ret) { if (nr_chars) { cr->buffer[nr_chars] = '\0'; - *line_r = cr->buffer; + *buff = cr->buffer; ret = 1; } else { - *line_r = NULL; + *buff = NULL; ret = 0; } }
Despite its name, libxl_xen_console_read_line() does not read a line, it fills the buffer with as many characters as fit. Update the documentation to reflect the real behaviour of the function. Rename line_r to avoid confusion since it is a pointer to an array of characters. Signed-off-by: Javi Merino <javi.merino@cloud.com> --- tools/libs/light/libxl_console.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)