Re: [PATCH V6 18/27] vfio-pci: refactor for cpr

[Steven Sistare]

Thanks for reviewing, and sorry for the delayed response, I just returned from 
vacation.

On 8/10/2021 12:53 PM, Alex Williamson wrote:
> On Fri,  6 Aug 2021 14:43:52 -0700
> Steve Sistare <steven.sistare@oracle.com> wrote:
> 
>> Export vfio_address_spaces and vfio_listener_skipped_section.
>> Add optional name arg to vfio_add_kvm_msi_virq.
>> Refactor vector use into a helper vfio_vector_init.
>> All for use by cpr in a subsequent patch.  No functional change.
> 
> Why is the name arg optional?  It seems really inconsistent to me that
> everything other than MSI/X uses this with a name, but MSI/X use NULL
> and in an entirely separate pre-save step we then iterate through all
> the {event,irq}fds to save them.  If we asked for a named notifier,
> shouldn't we go ahead and save it under that name at that time?  ie.
> 
> static int vfio_named_notifier_init(VFIOPCIDevice *vdev, EventNotifier *e,
>                                     const char *name, int nr)
> {
>     int ret, fd = load_event_fd(vdev, name, nr);
> 
>     if (fd >= 0) {
>         event_notifier_init_fd(e, fd);
>     } else {
>         ret = event_notifier_init(e, 0);
>         if (ret) {
>             return ret;
>         }
>         save_event_fd(vdev, name, nr, e);
>     }
>     return 0;
> }
> 
> Are we not doing this to avoid runtime overhead?

OK, I will delete the pre-save function and align the life-cycle of the fd and 
the event
notifier. (Currently the vfio-pci code does not call cpr_delete_fd.)

> In the process, maybe we can use more descriptive names than
> "interrupt", ex. "msi" or "msix".

I chose "interrupt" and "kvm_interrupt" to match the names of the corresponding 
VFIOMSIVector fields.  Ditto for intx-interrupt, intx-unmask, err, and req, with
minor differences.

> It also feels a bit forced to me that the entire fd saving uses {name,
> id} but vfio is the only caller that makes use of a non-zero id.
> Should we instead just wrap all the calls from vfio to append the id to
> the name so the common code can just use strcmp()?  Thanks,

I liked the simplification in the vfio code, but I will remove the id if you 
prefer,
and add g_autoptr and g_strdup_printf to each call site.

- Steve