Re: [PATCH V2 12/23] xen/ioreq: Remove "hvm" prefixes from involved function names

[Oleksandr <olekstysh@xxxxxxxxx>]

On 23.11.20 17:54, Paul Durrant wrote:

Hi Paul

> > -----Original Message-----
> > From: Oleksandr <olekstysh@xxxxxxxxx>
> > Sent: 23 November 2020 15:48
> > To: Jan Beulich <jbeulich@xxxxxxxx>; Paul Durrant <paul@xxxxxxx>
> > Cc: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>; Andrew Cooper 
> > <andrew.cooper3@xxxxxxxxxx>;
> > Roger Pau Monné <roger.pau@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; George Dunlap
> > <george.dunlap@xxxxxxxxxx>; Ian Jackson <iwj@xxxxxxxxxxxxxx>; Julien Grall 
> > <julien@xxxxxxx>; Stefano
> > Stabellini <sstabellini@xxxxxxxxxx>; Jun Nakajima <jun.nakajima@xxxxxxxxx>; 
> > Kevin Tian
> > <kevin.tian@xxxxxxxxx>; Julien Grall <julien.grall@xxxxxxx>; 
> > xen-devel@xxxxxxxxxxxxxxxxxxxx
> > Subject: Re: [PATCH V2 12/23] xen/ioreq: Remove "hvm" prefixes from involved 
> > function names
> >
> >
> > On 23.11.20 16:56, Jan Beulich wrote:
> >
> > Hi Jan, Paul
> >
> > > On 23.11.2020 15:39, Oleksandr wrote:
> > > > As it was agreed, below the list of proposed renaming (naming) within
> > > > current series.
> > > Thanks for compiling this. A couple of suggestions for consideration:
> > >
> > > > 1. Global (existing):
> > > > hvm_map_mem_type_to_ioreq_server     -> ioreq_server_map_mem_type
> > > > hvm_select_ioreq_server              -> ioreq_server_select
> > > > hvm_send_ioreq                       -> ioreq_send
> > > > hvm_ioreq_init                       -> ioreq_init
> > > ioreq_domain_init() (or, imo less desirable domain_ioreq_init())?
> > On Arm (for example) I see two variants are present:
> > 1. That starts with subsystem:
> > - tee_domain_init
> > - iommu_domain_init
> >
> >
> > 2. Where sybsystem in the middle:
> > - domain_io_init
> > - domain_vuart_init
> > - domain_vtimer_init
> >
> > If there is no rule, but a matter of taste then I would use
> > ioreq_domain_init(),
> > so arch_ioreq_init() wants to be arch_ioreq_domain_init().
> >
> > > > hvm_destroy_all_ioreq_servers        -> ioreq_server_destroy_all
> > > > hvm_all_ioreq_servers_add_vcpu       -> ioreq_server_add_vcpu_all
> > > > hvm_all_ioreq_servers_remove_vcpu    -> ioreq_server_remove_vcpu_all
> > > > hvm_broadcast_ioreq                  -> ioreq_broadcast
> > > > hvm_create_ioreq_server              -> ioreq_server_create
> > > > hvm_get_ioreq_server_info            -> ioreq_server_get_info
> > > > hvm_map_io_range_to_ioreq_server     -> ioreq_server_map_io_range
> > > > hvm_unmap_io_range_from_ioreq_server -> ioreq_server_unmap_io_range
> > > > hvm_set_ioreq_server_state           -> ioreq_server_set_state
> > > > hvm_destroy_ioreq_server             -> ioreq_server_destroy
> > > > hvm_get_ioreq_server_frame           -> ioreq_server_get_frame
> > > > hvm_ioreq_needs_completion           -> ioreq_needs_completion
> > > > hvm_mmio_first_byte                  -> ioreq_mmio_first_byte
> > > > hvm_mmio_last_byte                   -> ioreq_mmio_last_byte
> > > > send_invalidate_req                  -> ioreq_signal_mapcache_invalidate
> > > >
> > > > handle_hvm_io_completion             -> handle_io_completion
> > > For this one I'm not sure what to suggest, but I'm not overly happy
> > > with the name.
> > I also failed to find a better name. Probably ioreq_ or vcpu_ioreq_
> > prefix wants to be added here?
> >
> >
> > > > hvm_io_pending                       -> io_pending
> > > vcpu_ioreq_pending() or vcpu_any_ioreq_pending()?
> > I am fine with vcpu_ioreq_pending()
> ...in which case vcpu_ioreq_handle_completion() seems like a reasonable choice.
ok, will rename here ...


> > > > 2. Global (new):
> > > > arch_io_completion
and here arch_vcpu_ioreq_completion() (without handle in the middle).



> > > > arch_ioreq_server_map_pages
> > > > arch_ioreq_server_unmap_pages
> > > > arch_ioreq_server_enable
> > > > arch_ioreq_server_disable
> > > > arch_ioreq_server_destroy
> > > > arch_ioreq_server_map_mem_type
> > > > arch_ioreq_server_destroy_all
> > > > arch_ioreq_server_get_type_addr
> > > > arch_ioreq_init
> > > Assuming this is the arch hook of the similarly named function
> > > further up, a similar adjustment may then be wanted here.
> > Yes.
> >
> >
> > > > domain_has_ioreq_server
> > > >
> > > >
> > > > 3. Local (existing) in common ioreq.c:
> > > > hvm_alloc_ioreq_mfn               -> ioreq_alloc_mfn
> > > > hvm_free_ioreq_mfn                -> ioreq_free_mfn
> > > These two are server functions, so should imo be ioreq_server_...().
> > ok, but ...
> >
> >
> > > However, if they're static (as they're now), no distinguishing
> > > prefix is strictly necessary, i.e. alloc_mfn() and free_mfn() may
> > > be fine. The two names may be too short for Paul's taste, though.
> > > Some similar shortening may be possible for some or all of the ones
> > ... In general I would be fine with any option. However, using the
> > shortening rule for all
> > we are going to end up with single-word function names (enable, init, etc).
> > So I would prefer to leave locals as is (but dropping hvm prefixes of
> > course and
> > clarify ioreq_server_alloc_mfn/ioreq_server_free_mfn).
> >
> > Paul, Jan what do you think?
> I prefer ioreq_server_alloc_mfn/ioreq_server_free_mfn. The problem with 
> shortening is that function names become ambiguous within the source base and 
> hence harder to find.
Got it.


Thank you

--
Regards,

Oleksandr Tyshchenko