Re: [Qemu-devel] [PATCH 1/2] block/gluster: Handle changed glfs_ftruncate signature

[Niels de Vos]

On Mon, Mar 04, 2019 at 04:41:44PM +0000, Daniel P. Berrangé wrote:
> On Mon, Mar 04, 2019 at 05:21:02PM +0100, Niels de Vos wrote:
> > From: Prasanna Kumar Kalever <address@hidden>
> > 
> > New versions of Glusters libgfapi.so have an updated glfs_ftruncate()
> > function that returns additional 'struct stat' structures to enable
> > advanced caching of attributes. This is useful for file servers, not so
> > much for QEMU. Nevertheless, the API has changed and needs to be
> > adopted.
> > 
> > Signed-off-by: Prasanna Kumar Kalever <address@hidden>
> > Signed-off-by: Niels de Vos <address@hidden>
> > 
> > ---
> > v4: rebase to current master branch
> > v3: define old backwards compatible glfs_ftruncate() macro, from Eric Blake
> > v2: do a compile check as suggested by Eric Blake
> > ---
> >  block/gluster.c | 11 +++++++++--
> >  configure       | 18 ++++++++++++++++++
> >  2 files changed, 27 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/gluster.c b/block/gluster.c
> > index af64330211..86e5278524 100644
> > --- a/block/gluster.c
> > +++ b/block/gluster.c
> > @@ -20,6 +20,10 @@
> >  #include "qemu/option.h"
> >  #include "qemu/cutils.h"
> >  
> > +#ifdef CONFIG_GLUSTERFS_LEGACY_FTRUNCATE
> > +# define glfs_ftruncate(fd, offset, _u1, _u2) glfs_ftruncate(fd, offset)
> > +#endif
> 
> I don't much like this approach as it sets up a trapdoor. If future
> QEMU passes a non-NULL value for the 3rd/4th argument it will be
> silently ignored depending on glfs version built against which can
> result in incorrect behaviour at runtime. If we reverse it:
> 
>  #ifndef CONFIG_GLUSTERFS_LEGACY_FTRUNCATE
>  # define glfs_ftruncate(fd, offset) glfs_ftruncate(fd, offset, NULL, NULL)
>  #endif
> 
> then it ensures we can't silently do the wrong thing in future. Anyone
> who wants to use the 3rd/4th args will have to make an explicit effort
> to ensure it works correctly with old glfs APIs. An added benefit is
> that it avoids the following patch chunks.

Thanks for the suggestion. All makes sense and I'll update the patch
accordingly.

Niels