[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libvir] PATCH 1/4: QEMU driver char device support



"Daniel P. Berrange" <berrange redhat com> 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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]