> -----Original Message-----
> From: Oleksandr <olekst...@gmail.com>
> Sent: 23 November 2020 15:48
> To: Jan Beulich <jbeul...@suse.com>; Paul Durrant <p...@xen.org>
> Cc: Oleksandr Tyshchenko <oleksandr_tyshche...@epam.com>; Andrew Cooper 
> <andrew.coop...@citrix.com>;
> Roger Pau Monné <roger....@citrix.com>; Wei Liu <w...@xen.org>; George Dunlap
> <george.dun...@citrix.com>; Ian Jackson <i...@xenproject.org>; Julien Grall 
> <jul...@xen.org>; Stefano
> Stabellini <sstabell...@kernel.org>; Jun Nakajima <jun.nakaj...@intel.com>; 
> Kevin Tian
> <kevin.t...@intel.com>; Julien Grall <julien.gr...@arm.com>; 
> xen-devel@lists.xenproject.org
> 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.

> 
> >
> >> 2. Global (new):
> >> arch_io_completion
> >> 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.

  Paul


Reply via email to