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

Re: [libvirt] [PATCH] add sendevent command and related APIs



On 04/01/2011 03:51 AM, Lai Jiangshan wrote:
> Enable libvirt send some events to the guest.
> This command currently only supports NMI and key events.
> 
> Signed-off-by: Lai Jiangshan <laijs cn fujitsu com>

Would you mind resending this split into several patches?  For example,
see how
https://www.redhat.com/archives/libvir-list/2011-April/msg00052.html
divided a new API into 7 patches:

Michal Privoznik (7):
  screenshot: Defining the public API
  screenshot: Defining the internal API
  screenshot: Implementing the public API
  screenshot: Implementing the remote protocol
  screenshot: Expose the new API in virsh
  qemu: Implement the driver methods
  vbox: Implement the driver methods

By separating definition, public access, virsh use, and driver
implementation into separate patches, it becomes easier to review.  Each
division should still cleanly compile.  See also
http://libvirt.org/api_extension.html (although it predates some of the
recent changes such as adding libxl and dropping OpenNebula, it is still
a good reference).

> +++ b/src/libvirt.c
> @@ -5245,6 +5245,94 @@ error:
>  }
>  
>  /**
> + * virDomainSendEvnetNMI:

s/Evnet/Event/

> + * @domain: pointer to domain object, or NULL for Domain0
> + * @vcpu:   the virtual CPU id to send NMI to
> + *
> + * Send NMI to a special vcpu of the guest

s/special/specified/

> + *
> + * Returns 0 in case of success, -1 in case of failure.
> + */
> +
> +int virDomainSendEventNMI(virDomainPtr domain, unsigned int vcpu)

Absolutely must have a flags argument for future extension (even if the
only supported flags value is 0 for now).

> +
> +/**
> + * virDomainSendEventKey:
> + * @domain: pointer to domain object, or NULL for Domain0
> + * @key:    the string of key or key sequence
> + *
> + * Send a special key or key sequence to the guest
> + *
> + * Returns 0 in case of success, -1 in case of failure.
> + */
> +
> +int virDomainSendEventKey(virDomainPtr domain, const char *key)

Needs a flags argument.  Also, what is the format of a key sequence?
There's several layers of key events, from what a keyboard sends, to
what the OS driver encodes, to what X encodes, and so forth.  If the
format ever needs to include a NUL byte, then you need a length
parameter.  I'm worried that this is a bit underspecified.  Do we know
in advance which keys are acceptable (I'm assuming this is for SysRq
sequences), in which case an enum value might be better than a raw char*.

> +++ b/src/libvirt_public.syms
> @@ -434,6 +434,8 @@ LIBVIRT_0.9.0 {
>          virEventRunDefaultImpl;
>          virStorageVolDownload;
>          virStorageVolUpload;
> +        virDomainSendEventNMI;
> +        virDomainSendEventKey;
>  } LIBVIRT_0.8.8;

This is too late for the 0.9.0 feature freeze cutoff; when you split
this into multiple patches, can you rebase it to be targetting 0.9.1?

I haven't closely reviewed the rest of the patch yet, but think that the
overall idea of being able to inject NMI and SysRq input to the virtual
hardware of the guest is worth adding.

-- 
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


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