Re: [PATCH v2 4/5] progressmeter: protect with a mutex

[Stefan Hajnoczi]

On Tue, May 18, 2021 at 12:14:17PM +0200, Emanuele Giuseppe Esposito wrote:
> On 18/05/2021 12:00, Vladimir Sementsov-Ogievskiy wrote:
> > 18.05.2021 12:40, Emanuele Giuseppe Esposito wrote:
> > > Progressmeter is protected by the AioContext mutex, which
> > > is taken by the block jobs and their caller (like blockdev).
> > > 
> > > We would like to remove the dependency of block layer code on the
> > > AioContext mutex, since most drivers and the core I/O code are already
> > > not relying on it.
> > > 
> > > Create a new C file to implement the ProgressMeter API, but keep the
> > > struct as public, to avoid forcing allocation on the heap.
> > > 
> > > Also add a mutex to be able to provide an accurate snapshot of the
> > > progress values to the caller.
> > > 
> > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > 
> > patch changed a lot, so you can't keep Stefan's r-b. r-b should be kept
> > if patch is unchanged.
> 
> Sorry, my bad. Will remove it, if we keep these changes (see below).

I took a look again:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Regarding hiding the struct, in a library with a public API the authors
need to be very careful about exposing struct fields in order to prevent
ABI breakage or applications making use of internal fields.

However, in QEMU we're less strict since we have full control over the
codebase (including internal API consumers). If other factors outweigh
the need for strict encapsulation, then you can put the struct
definition in the header file. Grepping for "private" in QEMU shows lots
of examples and there are probably many more without an explicit
"private" comment. Code reviewers can reject patches that touch private
struct fields or patches can be submitted to remove existing access to
private fields, so we don't have the limitations that library authors
have.

Stefan

signature.asc
Description: PGP signature