Re: [Qemu-devel] [PATCH] Add info commands for serial/parallel devices

[Anthony Liguori]

Thiemo Seufer wrote:
> Anthony Liguori wrote:
>   
> > Howdy,
> >
> > The following patch adds an info serial and an info parallel command.  
> > Besides providing useful information (especially for the serial port), 
> > it provides a method for management tools to connect to a running VM and 
> > what character devices the serial/parallel ports have been redirected to.
> >
> > The format of the info is similar to that of info block.
> >     
> [snip]
>   
> > diff -r 18e99d1e8814 vl.c
> > --- a/vl.c      Sat Mar 03 21:18:48 2007 -0600
> > +++ b/vl.c      Sat Mar 03 21:33:07 2007 -0600
> > @@ -2884,66 +2884,73 @@ CharDriverState *qemu_chr_open(const cha
> >  CharDriverState *qemu_chr_open(const char *filename)
> >  {
> >      const char *p;
> > +    CharDriverState *chr;
> >  
> >      if (!strcmp(filename, "vc")) {
> > -        return text_console_init(&display_state);
> > +        chr = text_console_init(&display_state);
> >      } else if (!strcmp(filename, "null")) {
> > -        return qemu_chr_open_null();
> > +        chr = qemu_chr_open_null();
> >      } else 
> >      if (strstart(filename, "tcp:", &p)) {
> > -        return qemu_chr_open_tcp(p, 0, 0);
> > +        chr = qemu_chr_open_tcp(p, 0, 0);
> >      } else
> >      if (strstart(filename, "telnet:", &p)) {
> > -        return qemu_chr_open_tcp(p, 1, 0);
> > +        chr = qemu_chr_open_tcp(p, 1, 0);
> >      } else
> >      if (strstart(filename, "udp:", &p)) {
> > -        return qemu_chr_open_udp(p);
> > +        chr = qemu_chr_open_udp(p);
> >      } else
> >      if (strstart(filename, "mon:", &p)) {
> >          CharDriverState *drv = qemu_chr_open(p);
> >          if (drv) {
> >              drv = qemu_chr_open_mux(drv);
> >              monitor_init(drv, !nographic);
> > -            return drv;
> > -        }
> > -        printf("Unable to open driver: %s\n", p);
> > -        return 0;
> > +            chr = drv;
> > +        } else {
> > +           printf("Unable to open driver: %s\n", p);
> > +           return 0;
> > +       }
> >      } else
> >  #ifndef _WIN32
> >      if (strstart(filename, "unix:", &p)) {
> > -       return qemu_chr_open_tcp(p, 0, 1);
> > +       chr = qemu_chr_open_tcp(p, 0, 1);
> >      } else if (strstart(filename, "file:", &p)) {
> > -        return qemu_chr_open_file_out(p);
> > +        chr = qemu_chr_open_file_out(p);
> >      } else if (strstart(filename, "pipe:", &p)) {
> > -        return qemu_chr_open_pipe(p);
> > +        chr = qemu_chr_open_pipe(p);
> >      } else if (!strcmp(filename, "pty")) {
> > -        return qemu_chr_open_pty();
> > +        chr = qemu_chr_open_pty();
> >      } else if (!strcmp(filename, "stdio")) {
> > -        return qemu_chr_open_stdio();
> > +        chr = qemu_chr_open_stdio();
> >      } else 
> >  #endif
> >  #if defined(__linux__)
> >      if (strstart(filename, "/dev/parport", NULL)) {
> > -        return qemu_chr_open_pp(filename);
> > +        chr = qemu_chr_open_pp(filename);
> >      } else 
> >      if (strstart(filename, "/dev/", NULL)) {
> > -        return qemu_chr_open_tty(filename);
> > +        chr = qemu_chr_open_tty(filename);
> >      } else 
> >  #endif
> >  #ifdef _WIN32
> >      if (strstart(filename, "COM", NULL)) {
> > -        return qemu_chr_open_win(filename);
> > +        chr = qemu_chr_open_win(filename);
> >      } else
> >      if (strstart(filename, "pipe:", &p)) {
> > -        return qemu_chr_open_win_pipe(p);
> > +        chr = qemu_chr_open_win_pipe(p);
> >      } else
> >      if (strstart(filename, "file:", &p)) {
> > -        return qemu_chr_open_win_file_out(p);
> > -    }
> > +        chr = qemu_chr_open_win_file_out(p);
> > +    } else
> >  #endif
> >      {
> >          return NULL;
> >      }
> > +
> > +    if (chr)
> > +       chr->filename = strdup(filename);
> > +
> > +    return chr;
> >     
>
> Why is this part needed?
>   

So that info seral|parallel can spit out the name used to create open 
the device.

It has to be strdup()'d b/c it's passed in as a const char.  There's no 
guarantee that memory will stay around.

> >  }
> >  
> >  void qemu_chr_close(CharDriverState *chr)
> > diff -r 18e99d1e8814 vl.h
> > --- a/vl.h      Sat Mar 03 21:18:48 2007 -0600
> > +++ b/vl.h      Sat Mar 03 21:33:07 2007 -0600
> > @@ -307,6 +307,7 @@ typedef struct CharDriverState {
> >      void *opaque;
> >      int focus;
> >      QEMUBH *bh;
> > +    char *filename;
> >  } CharDriverState;
> >     
>
> const char * ?
>   

To me, const char * always implies that you don't own the memory.  This 
is helped by the fact that free doesn't take a const void * and newer 
GCC's will complain if you free() a const char *.

Regards,

Anthony Liguori

> Thiemo
>
>