"Daniel P. Berrange" <berrange@xxxxxxxxxx> wrote:
> On Fri, Apr 18, 2008 at 09:25:28PM +0100, Daniel P. Berrange wrote:
>> This supports the serial/character devices in QEMU. It is complete
>> except for fact that I don't extract the TTY path for type='pty'.
>> This needs addressing before merging
>
> This patch now includes the ability to extract the TTY path for
> serial and parallel devices, so we finally have 'virsh console'
> working for QEMU. THat said, plain QEMU/KVM has busted serial
> console which translates \r\n into \n\n, but I've sent a patch
> for that upstream
Nice!
...
> Index: src/qemu_conf.c
> ===================================================================
...
> +static int qemudGenerateXMLChar(virBufferPtr buf,
> + const struct qemud_vm_chr_def *dev,
> + const char *type)
> +{
> + const char *const types[] = {
> + "null",
> + "vc",
> + "pty",
> + "dev",
> + "file",
> + "pipe",
> + "stdio",
> + "udp",
> + "tcp",
> + "unix"
> + };
> + /*verify(ARRAY_CARDINALITY(types) == QEMUD_CHR_SRC_TYPE_LAST);*/
A couple days ago I relaxed the copyright on the gnulib module to
LGPLv2+, so you can go ahead and uncomment that, #include "verify.h",
and add "verify" to the list in bootstrap.
> Index: src/qemu_driver.c
> ===================================================================
...
> -static int qemudExtractMonitorPath(const char *haystack, char *path, int pathmax) {
> +static int qemudExtractMonitorPath(const char *haystack,
> + size_t *offset,
> + char *path, int pathmax) {
Thanks for shortening long lines ;-)
> static const char needle[] = "char device redirected to";
> char *tmp;
>
> - if (!(tmp = strstr(haystack, needle)))
> + /* First look for our magic string */
> + if (!(tmp = strstr(haystack + *offset, needle)))
> return -1;
>
> + /* Grab all the trailing data */
> strncpy(path, tmp+sizeof(needle), pathmax-1);
That should be sizeof(needle)-1.
Otherwise, if someone nasty gave you input ending with
"char device redirected to", the strncpy above would start
reading just past the NUL at the end of "haystack".
> path[pathmax-1] = '\0';
>
> - while (*path) {
> - /*
> - * The monitor path ends at first whitespace char
> - * so lets search for it & NULL terminate it there
> - */
> - if (isspace(*path)) {
> - *path = '\0';
> + /*
> + * And look for first whitespace character and nul terminate
> + * to mark end of the pty path
> + */
> + tmp = path;
> + while (*tmp) {
> + if (isspace(*tmp)) {
Since "tmp" has type "char", this causes trouble in an environment
where "char" is a signed type. When *tmp is larger than 127, it gets
sign-extended, and isspace can misbehave on the large negative number
(isspace is not defined for such values). Instead, do it like this:
if (isspace(*(unsigned char *)tmp)) {
or better, using the to_uchar function (from coreutils):
if (isspace(to_uchar(tmp))) {
/* Convert a possibly-signed character to an unsigned character. This is
a bit safer than casting to unsigned char, since it catches some type
errors that the cast doesn't. */
static inline unsigned char to_uchar (char ch) { return ch; }
I just happened upon this one, but have audited the rest of the code
for similar problems. To get an idea of the size of this task,
I got the list of is* functions from the man page and did this:
(tolower and toupper have the same limitation)
re=$(man isspace|grep is.....,.is|sed 's/ -.*//'|tr -s ', \n' \| \
|sed 's/^|//;s/|$//')
git grep -E "\b($re|tolower|toupper)\b"
Here's the output:
ChangeLog: Unlike in qemud.c, here we allow trailing "isspace", and in
ChangeLog: * src/virsh.c: Remove use of _GNU_SOURCE / isblank.
src/nodeinfo.c: while (*buf && isspace(*buf))
src/nodeinfo.c: while (*buf && isspace(*buf))
src/nodeinfo.c: && (*p == '\0' || *p == '.' || isspace(*p)))
src/nodeinfo.c: while (*buf && isspace(*buf))
src/nodeinfo.c: && (*p == '\0' || isspace(*p))
src/qemu_driver.c: if (isspace(*path)) {
src/sexpr.c: while (*ptr && !isspace(*ptr) && *ptr != ')' && *ptr != '
src/stats_linux.c: if (!isdigit(path[4]) || path[4] == '0' ||
src/stats_linux.c: if (p && (!isdigit(*p) || *p == '0' ||
src/stats_linux.c: if (!isdigit(path[3]) || path[3] == '0' ||
src/util.c:#define TOLOWER(Ch) (isupper (Ch) ? tolower (Ch) : (Ch))
src/util.c: while (*p == '0' && isxdigit (p[1]))
src/util.c: while (*q == '0' && isxdigit (q[1]))
src/util.c: if (!isxdigit(*str))
src/virsh.c: if (!isdigit (cpulist[i])) {
src/virsh.c: else if (!isdigit (cpulist[i])) {
src/virsh.c: && isalnum((unsigned char) *(p + 2))) {
So I've just posted a patch to fix those.
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list