Re: [Xen-devel] 32-on-64: pvfb issue

[Markus Armbruster <armbru@xxxxxxxxxx>]

Gerd Hoffmann <kraxel@xxxxxxx> writes:

>   Hi,
>
>>> @@ -560,10 +560,10 @@ int xenfb_attach_dom(struct xenfb *xenfb
>>>     if (xenfb_wait_for_frontend_initialised(&xenfb->kbd) < 0)
>>>             goto error;
>>>  
>>> -   if (xenfb_bind(&xenfb->fb) < 0)
>>> -           goto error;
>>>     if (xenfb_bind(&xenfb->kbd) < 0)
>>>             goto error;
>>> +   if (xenfb_bind(&xenfb->fb) < 0)
>>> +           goto error;
>>>  
>>>     if (xenfb_xs_scanf1(xsh, xenfb->fb.otherend, "feature-update",
>>>                         "%d", &val) < 0)
>> 
>> Why is this patch hunk necessary?
>
> Oh, forgot to mention that, sorry.  Only vfb has a "protocol" node, vkbd
> hasn't.  So binding fb last makes sure we have the protocol filled
> correctly in the struct.
>
> cheers,
>   Gerd

Unclean!

You put protocol[] into xenfb_private, which means it's shared between
vfb and vkbd.  That's defensible.  However, you really shouldn't read
it in xenfb_bind() then.  That reads it both from vfb (where it is
valid) and vkbd (where it is currently undefined), and the one read
last wins.

Reading it next to reading feature-update would be much cleaner.

Alternatively, put protocol[] into xenfb_device.

If you insist on keeping it the way it is, you really need a comment.
But cleaning it up should be less work than explaining it.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel