[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



> -----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.

> 
> >
> >> 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




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.