Re: [Xen-devel] [PATCH v4] add support for libvirt-like channels

[Dave Scott <Dave.Scott@xxxxxxxxxx>]

On 7 Aug 2014, at 15:26, Stefano Stabellini <Stefano.Stabellini@xxxxxxxxxxxxx> 
wrote:

> On Thu, 7 Aug 2014, David Vrabel wrote:
>> On 07/08/14 14:37, Dave Scott wrote:
>>> 
>>> On 28 Jul 2014, at 15:22, David Vrabel <david.vrabel@xxxxxxxxxx> wrote:
>>> 
>>>> On 22/07/14 16:05, David Scott wrote:
>>>>> 
>>>>> Note: I've seen a problem with some Linux console frontends which attempt
>>>>> to overwrite the read-only key 'type' (= xenconsoled|ioemu) in the 
>>>>> frontend
>>>>> directory. [ this key is already present, it's not added by these patches 
>>>>> ]
>>>>> Since 'type' refers to the toolstack's choice of implementation service
>>>>> (which is none of the guest's business) I think the read/only permissions
>>>>> are right. The location of the key is not ideal (it should be in the
>>>>> backend directory IMHO) but this is the case with several other keys 
>>>>> located
>>>>> in the frontend such as 'limit' and 'tty'. I think this is a Linux 
>>>>> frontend
>>>>> bug which should be fixed there. These patches work with Mirage VMs and
>>>>> with Linux if I workaround the permissions by giving the guest read/write
>>>>> to the 'type' key. 
>>>> 
>>>> Which Linux front ends?
>>> 
>>> I just tested a Debian jessie with
>>> 
>>>  # uname -a
>>>  Linux jessie 3.13-6-686-pae #1 SMP Debian 3.13.5-1 (2014-03-04) i686 
>>> GNU/Linux
>>> 
>>> In my xenstored access log I see:
>>> 
>>>  Aug  7 14:28:30 localhost xenstored:  D40.6        write     
>>> device/console/1/ring-ref 8
>>>  Aug  7 14:28:30 localhost xenstored:  D40.6        write     
>>> device/console/1/port 10
>>>  Aug  7 14:28:30 localhost xenstored:  D40.6        write     
>>> device/console/1/type ioemu
>>> 
>>> — I think the last line is suspicious.
>> 
>> This was introduced by Stefano in 3.4 with 02e19f9c7cac (hvc_xen:
>> implement multiconsole support).
>> 
>> Stefano, can you advise on whether it it safe to remove the write of the
>> "type" key or whether we need to make it conditional on it being absent.
> 
> I think that the original idea was that since only ioemu backends (QEMU)
> are capable of handling multiple consoles and the new xenstore based
> console protocol, then the "type" had to be "ioemu".
> 
> I agree that the type key has no business being in the frontend
> directory.
> I also agree that the frontend shouldn't be writing it, since the
> backend would have already been started anyway.

Would you like me to move the key to the backend directory? I could leave a 
writable (but unused) key in the frontend directory for backwards 
compatibility. Or should I leave this alone?

> 
> I think it would be OK to:
> 
> ---
> xen_hvc: no reason to write the type key on xenstore
> 
> The backend type is chosen by the toolstack. Regardless the frontend
> should not care.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> 
> diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
> index 636c9ba..b8491f5 100644
> --- a/drivers/tty/hvc/hvc_xen.c
> +++ b/drivers/tty/hvc/hvc_xen.c
> @@ -402,9 +402,6 @@ static int xencons_connect_backend(struct xenbus_device 
> *dev,
>                           evtchn);
>       if (ret)
>               goto error_xenbus;
> -     ret = xenbus_printf(xbt, dev->nodename, "type", "ioemu");
> -     if (ret)
> -             goto error_xenbus;
>       ret = xenbus_transaction_end(xbt, 0);
>       if (ret) {
>               if (ret == -EAGAIN)

This looks good to me :-)

Cheers,
Dave


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