Re: [Xen-devel] [PATCH v6 for-4.5 5/5] xl: add support for 'channels'

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, Sep 24, 2014 at 09:48:05PM +0100, David Scott wrote:
[...]
> +=item C<name=NAME>
> +
> +Specify the string name for this device. This parameter is mandatory.
> +This should be a well-known name for the specific application (e.g.
> +guest agent) and should be used by the frontend to connect the
> +application to the right channel device. There is no formal registry
> +of channel names, so application authors are encouraged to make their
> +names unique by including domain name and version number in the string
> +(e.g. org.mydomain.guestagent.1).
> +
> +=item C<connection=CONNECTION>
> +
> +Specify how the backend will be implemented. This following options are
> +available:
> +
> +=over 4
> +
> +=item B<connection=SOCKET>:
> +

I don't think you need that ":"

> +The backend will bind a Unix domain socket (at the path given by
> +B<path=PATH>), call listen and accept connections. The backend will proxy
> +data between the channel and the connected socket.
> +
> +=item B<connection=PTY>:
> +

Ditto.

> +The backend will create a pty and proxy data between the channel and the
> +master device. The command B<xl channel-list> can be used to discover the
> +assigned slave device.
> +
> +=back
> +
> +=back
> +
>  =item B<pci=[ "PCI_SPEC_STRING", "PCI_SPEC_STRING", ... ]>
>  
>  Specifies the host PCI devices to passthrough to this guest. Each 
> B<PCI_SPEC_STRING>
> diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> index f1e95db..35aa3a4 100644
> --- a/docs/man/xl.pod.1
> +++ b/docs/man/xl.pod.1
> @@ -1215,6 +1215,16 @@ List virtual network interfaces for a domain.
>  
>  =back
>  
> +=head2 CHANNEL DEVICES
> +
> +=over 4
> +
> +=item B<channel-list> I<domain-id>
> +
> +List virtual channel interfaces for a domain.
> +
> +=back
> +
>  =head2 VTPM DEVICES
>  
>  =over 4
> diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
> index 10a2e66..afc5f8b 100644
> --- a/tools/libxl/xl.h
> +++ b/tools/libxl/xl.h
> @@ -78,6 +78,7 @@ int main_top(int argc, char **argv);
>  int main_networkattach(int argc, char **argv);
>  int main_networklist(int argc, char **argv);
>  int main_networkdetach(int argc, char **argv);
> +int main_channellist(int argc, char **argv);
>  int main_blockattach(int argc, char **argv);
>  int main_blocklist(int argc, char **argv);
>  int main_blockdetach(int argc, char **argv);
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index d6f311f..89a8697 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -530,7 +530,7 @@ static void split_string_into_string_list(const char *str,
>  
>      s = strdup(str);
>      if (s == NULL) {
> -        fprintf(stderr, "unable to allocate memory to parse bootloader 
> args\n");
> +        fprintf(stderr, "unable to allocate memory to split string\n");
>          exit(-1);
>      }
>  
> @@ -546,7 +546,7 @@ static void split_string_into_string_list(const char *str,
>  
>      sl = malloc((nr+1) * sizeof (char *));
>      if (sl == NULL) {
> -        fprintf(stderr, "unable to allocate memory for bootloader args\n");
> +        fprintf(stderr, "unable to allocate memory to split string\n");
>          exit(-1);
>      }
>  
> @@ -567,7 +567,6 @@ static void split_string_into_string_list(const char *str,
>     and look for CTYPE in libxl_internal.h */
>  typedef int (*char_predicate_t)(const int c);
>  
> -static void trim(char_predicate_t predicate, const char *input, char 
> **output) __attribute__ ((unused));
>  static void trim(char_predicate_t predicate, const char *input, char 
> **output)
>  {
>      char *p, *q, *tmp;
> @@ -593,10 +592,6 @@ static void trim(char_predicate_t predicate, const char 
> *input, char **output)
>  static int split_string_into_pair(const char *str,
>                                    const char *delim,
>                                    char **a,
> -                                  char **b) __attribute__ ((unused));
[...]
> +            switch (chn->connection){
> +            case LIBXL_CHANNEL_CONNECTION_UNKNOWN:
> +                fprintf(stderr, "channel has unknown 'connection'\n");
> +                exit(1);
> +            case LIBXL_CHANNEL_CONNECTION_SOCKET:
> +                if (!path) {
> +                    fprintf(stderr, "channel connection 'socket' requires 
> path=..\n");
> +                    exit(1);
> +                }
> +                chn->u.socket.path = xstrdup(path);
> +                break;

Don't need to do anything for 'PTY' connection? If so, please as a
comment here.

> +            default:
> +                break;

So this branch covers 'PTY' case? If so, please document, if not it
should have exit(1).

I can see from the code at this point chn->connection is limited to one
of the three types defined so far, but I would rather be explicit. That
is, have 

   case LIBXL_CHANNEL_CONNECTION_PTY:
       /* nothing to do */
       break;
   default:
       fprintf("something's wrong\n");
       exit(1);

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel