Re: [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest

["Lukáš Hrázký" <lhrazky@redhat.com>]

Hello,

On Mon, 2018-10-22 at 13:46 +0200, Lukáš Hrázký wrote:
> Hello,
> 
> On Thu, 2018-10-18 at 03:16 -0400, Frediano Ziglio wrote:
> > > 
> > > Adds two functions to let QEMU provide information to identify graphics
> > > devices and their monitors in the guest:
> > > 
> > > * device address - The path identifying the device on the system (e.g. PCI
> > >   path):
> > >   spice_qxl_set_device_address(...)
> > > 
> > > * device display ID - The index of the monitor on the graphics device:
> > >   spice_qxl_monitor_set_device_display_id(...)
> > 
> > This seems to indicate that this device is bound in some way to the previous
> > information but having 2 APIs make more fragile, potentially one could
> > call a function and not the other or in different order or mismatch them.
> 
> Not sure what you mean by "previous information".
> 
> Yes, you need to call both functions... The order doesn't matter and
> you can always send wrong data in a single function as well. I agree in
> this regard a single function would be better but don't find it a big
> issue. Note that the functions are called in different places in QEMU
> too.
> 
> ...
> 
> > > +SPICE_GNUC_VISIBLE
> > > +void spice_qxl_set_device_address(QXLInstance *instance, const char
> > > *device_address)
> > > +{
> > > 
> > > ...
> > > 
> > > + */
> > > +SPICE_GNUC_VISIBLE
> > > +void spice_qxl_monitor_set_device_display_id(QXLInstance *instance,
> > > +                                             uint32_t monitor_id,
> > > +                                             uint32_t device_display_id)
> > 
> > I still don't understand why, as suggested by Gerd, we need another function
> > instead of 2 additional parameters to the above API specifying start and
> > number, this API looks much more prone to errors.
> 
> Possibly. I (and I think Jonathon too?) didn't like the start and
> number API, this was proposed as an alternative.

this topic of the API functions should be resolved. I think the options
are:


1. Keep the two functions proposed in this patch:

void spice_qxl_set_device_address(QXLInstance *instance,
                                  const char *device_address);

void spice_qxl_monitor_set_device_display_id(QXLInstance *instance,
                                             uint32_t monitor_id,
                                             uint32_t device_display_id);


2. Have a single function as follows:

void spice_qxl_set_device_info(QXLInstance *instance,
                               const char *device_address,
                               uint32_t device_display_id_start,
                               uint32_t device_display_id_count);


The semantics of the single function in 2. (with a sort-of placeholder
name) would be setting the device_address for the QXLInstance as well
as setting a mapping of monitor IDs to device display IDs of
[0, count-1] -> [start, start+count-1].

The usage of 1. in QEMU is what's in patch 2/2. The usage of 2. would
mean calling the new single function in the two places where
spice_qxl_monitor_set_device_display_id() is now called in patch 2/2.
The code that composes the device_address would need to be put in a
function somewhere in QEMU (not sure where yet) that can then be called
from the two places where it's needed.

I'm still somewhat more inclined to keep the two functions, as their
API seems a bit cleaner to me and you can call/not call a single
function in a wrong way too. Frediano prefers a single function.
Jonathon, Gerd?

Thanks,
Lukas