Re: [RFC v4 1/9] block: add block layer APIs resembling Linux ZonedBlockDevice ioctls.

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3096 bytes --]

On Wed, Jul 13, 2022 at 08:51:45AM +0800, Sam Li wrote:
> Stefan Hajnoczi <stefanha@redhat.com> 于2022年7月12日周二 23:49写道：
> >
> > On Tue, Jul 12, 2022 at 10:13:37AM +0800, Sam Li wrote:
> > > diff --git a/block/file-posix.c b/block/file-posix.c
> > > index 48cd096624..e7523ae2ed 100644
> > > --- a/block/file-posix.c
> > > +++ b/block/file-posix.c
> > > @@ -67,6 +67,7 @@
> > >  #include <sys/param.h>
> > >  #include <sys/syscall.h>
> > >  #include <sys/vfs.h>
> > > +#include <linux/blkzoned.h>
> > >  #include <linux/cdrom.h>
> > >  #include <linux/fd.h>
> > >  #include <linux/fs.h>
> > > @@ -216,6 +217,13 @@ typedef struct RawPosixAIOData {
> > >              PreallocMode prealloc;
> > >              Error **errp;
> > >          } truncate;
> > > +        struct {
> > > +            int64_t *nr_zones;
> > > +            BlockZoneDescriptor *zones;
> > > +        } zone_report;
> > > +        struct {
> > > +            zone_op op;
> > > +        } zone_mgmt;
> > >      };
> > >  } RawPosixAIOData;
> > >
> > > @@ -1801,6 +1809,130 @@ static off_t copy_file_range(int in_fd, off_t *in_off, int out_fd,
> > >  }
> > >  #endif
> > >
> >
> > Are the functions below within #ifdef __linux__?
> 
> Maybe add them later?

When using #ifdefs you usually need to apply them consistently to avoid
compiler errors or warnings.

For example:

  static void foo(void) { ... }

  #ifdef __linux__
  static BlockDriver ... = {
      .foo = foo,
  };
  #endif /* __linux__ */

On non-Linux hosts the compiler only sees foo() but it's unused, so it a
warning about an unused function will be displayed.

And the other way around results in a compiler error:

  #ifdef __linux__
  static void foo(void) { ... }
  #endif /* __linux__ */

  static BlockDriver ... = {
      .foo = foo,
  };

On non-Linux hosts the compiler doesn't see the definition of foo() that
the BlockDriver struct requires and compilation fails with an error.

There will be no errors on your machine if you avoid #ifdefs everywhere,
but reviewers will be worried about it and Continuous Integration tests
will fail to build on non-Linux hosts.

I would put the #ifdefs in place from the start.

> > > +static int coroutine_fn raw_co_zone_report(BlockDriverState *bs, int64_t offset,
> > > +                                           int64_t *nr_zones,
> > > +                                           BlockZoneDescriptor *zones) {
> > > +    BDRVRawState *s = bs->opaque;
> > > +    RawPosixAIOData acb;
> > > +
> > > +    acb = (RawPosixAIOData) {
> > > +        .bs         = bs,
> > > +        .aio_fildes = s->fd,
> > > +        .aio_type   = QEMU_AIO_IOCTL,
> >
> > Please define QEMU_AIO_ZONE_REPORT, QEMU_AIO_ZONE_MGMT, etc values for
> > these new operations instead of reusing QEMU_AIO_IOCTL, which is meant
> > for generic passthrough ioctls.

After looking again I think .aio_type isn't used in this code path. You
can skip initializing this field if you want and no new enum constants
are needed.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]