[Qemu-devel] PATCH: add missing (dummy) raw_aio_ioctl() for FreeBSD case

[Qemu-devel] PATCH: add missing (dummy) raw_aio_ioctl() for FreeBSD case

[Juergen Lock]

Assuming this never actually gets called (yet?), I just added a dummy:

Index: qemu/block-raw-posix.c
@@ -1323,6 +1323,16 @@
 {
     return -ENOTSUP;
 }
+
+#ifdef CONFIG_AIO
+static BlockDriverAIOCB *raw_aio_ioctl(BlockDriverState *bs,
+        unsigned long int req, void *buf,
+        BlockDriverCompletionFunc *cb, void *opaque)
+{
+    return NULL;
+}
+#endif
+
 #else /* !linux && !FreeBSD */
 
 static int fd_open(BlockDriverState *bs)

Signed-off-by: Juergen Lock <nox@jelal.kn-bremen.de>

Re: [Qemu-devel] PATCH: add missing (dummy) raw_aio_ioctl() for FreeBSD case

[Anthony Liguori]

Juergen Lock wrote:
> Assuming this never actually gets called (yet?), I just added a dummy:
>   

I'd suggest not --disable-aio on FreeBSD.

Certainly, FreeBSD has a working pthread, right?

Regards,

Anthony Liguori

Re: [Qemu-devel] PATCH: add missing (dummy) raw_aio_ioctl() for FreeBSD case

[M. Warner Losh]

In message: <49FB55ED.7050005@codemonkey.ws>
            Anthony Liguori <anthony@codemonkey.ws> writes:
: Juergen Lock wrote:
: > Assuming this never actually gets called (yet?), I just added a dummy:
: >   
: 
: I'd suggest not --disable-aio on FreeBSD.
: 
: Certainly, FreeBSD has a working pthread, right?

Yes...

Warner

Re: [Qemu-devel] PATCH: add missing (dummy) raw_aio_ioctl() for FreeBSD case

[Juergen Lock]

On Fri, May 01, 2009 at 02:18:52PM -0600, M. Warner Losh wrote:
> In message: <49FB55ED.7050005@codemonkey.ws>
>             Anthony Liguori <anthony@codemonkey.ws> writes:
> : Juergen Lock wrote:
> : > Assuming this never actually gets called (yet?), I just added a dummy:
> : >   
> : 
> : I'd suggest not --disable-aio on FreeBSD.
> : 
> : Certainly, FreeBSD has a working pthread, right?
> 
> Yes...

Oh, we are using aio afaict, at least configure says
	...
	AIO support       yes
	...
- but isn't this raw_aio_ioctl() only used for SG_IO i.e. scsi passthru?
(which isn't implemented for FreeBSD yet because doing direct scsi
commands from userland works differently there than on linux.)

 Or am I confused? :)

	Juergen

Re: [Qemu-devel] PATCH: add missing (dummy) raw_aio_ioctl() for FreeBSD case

[Christoph Hellwig]

On Fri, May 01, 2009 at 11:19:18PM +0200, Juergen Lock wrote:
> Oh, we are using aio afaict, at least configure says
> 	...
> 	AIO support       yes
> 	...
> - but isn't this raw_aio_ioctl() only used for SG_IO i.e. scsi passthru?
> (which isn't implemented for FreeBSD yet because doing direct scsi
> commands from userland works differently there than on linux.)
> 
>  Or am I confused? :)

You're not.  Currently aio_ioctl is only used for SG_IO.  But I don't
think wiring up for freebsd would hurt.  The actual posix-aio-compat.c
code for it is compiled on freebsd anyway, so hooking it up is easy,
and you're prepared if other users pop up.

Re: [Qemu-devel] PATCH: add missing (dummy) raw_aio_ioctl() for FreeBSD case

[Juergen Lock]

On Fri, May 01, 2009 at 11:52:26PM +0200, Christoph Hellwig wrote:
> On Fri, May 01, 2009 at 11:19:18PM +0200, Juergen Lock wrote:
> > Oh, we are using aio afaict, at least configure says
> > 	...
> > 	AIO support       yes
> > 	...
> > - but isn't this raw_aio_ioctl() only used for SG_IO i.e. scsi passthru?
> > (which isn't implemented for FreeBSD yet because doing direct scsi
> > commands from userland works differently there than on linux.)
> > 
> >  Or am I confused? :)
> 
> You're not.  Currently aio_ioctl is only used for SG_IO.  But I don't
> think wiring up for freebsd would hurt.  The actual posix-aio-compat.c
> code for it is compiled on freebsd anyway, so hooking it up is easy,
> and you're prepared if other users pop up.

Alright I can do that too...  Tho in that case I'd say raw_ioctl() needs
to be hooked up too:

Index: qemu/block-raw-posix.c
@@ -1321,8 +1321,44 @@
 
 static int raw_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
 {
-    return -ENOTSUP;
+    BDRVRawState *s = bs->opaque;
+
+    return ioctl(s->fd, req, buf);
+}
+
+#ifdef CONFIG_AIO
+static BlockDriverAIOCB *raw_aio_ioctl(BlockDriverState *bs,
+        unsigned long int req, void *buf,
+        BlockDriverCompletionFunc *cb, void *opaque)
+{
+    BDRVRawState *s = bs->opaque;
+    RawAIOCB *acb;
+
+    if (fd_open(bs) < 0)
+        return NULL;
+
+    acb = qemu_aio_get(bs, cb, opaque);
+    if (!acb)
+        return NULL;
+    acb->aiocb.aio_fildes = s->fd;
+    acb->aiocb.ev_signo = SIGUSR2;
+    acb->aiocb.aio_offset = 0;
+    acb->aiocb.aio_flags = 0;
+
+    acb->next = posix_aio_state->first_aio;
+    posix_aio_state->first_aio = acb;
+
+    acb->aiocb.aio_ioctl_buf = buf;
+    acb->aiocb.aio_ioctl_cmd = req;
+    if (qemu_paio_ioctl(&acb->aiocb) < 0) {
+        raw_aio_remove(acb);
+        return NULL;
+    }
+
+    return &acb->common;
 }
+#endif
+
 #else /* !linux && !FreeBSD */
 
 static int fd_open(BlockDriverState *bs)

Re: [Qemu-devel] PATCH: add missing (dummy) raw_aio_ioctl() for FreeBSD case

[Christoph Hellwig]

On Sat, May 02, 2009 at 04:30:45PM +0200, Juergen Lock wrote:
>  
>  static int raw_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
>  {
> -    return -ENOTSUP;
> +    BDRVRawState *s = bs->opaque;
> +
> +    return ioctl(s->fd, req, buf);
> +}
> +
> +#ifdef CONFIG_AIO
> +static BlockDriverAIOCB *raw_aio_ioctl(BlockDriverState *bs,
> +        unsigned long int req, void *buf,
> +        BlockDriverCompletionFunc *cb, void *opaque)
> +{
> +    BDRVRawState *s = bs->opaque;
> +    RawAIOCB *acb;
> +
> +    if (fd_open(bs) < 0)
> +        return NULL;
> +
> +    acb = qemu_aio_get(bs, cb, opaque);
> +    if (!acb)
> +        return NULL;
> +    acb->aiocb.aio_fildes = s->fd;
> +    acb->aiocb.ev_signo = SIGUSR2;
> +    acb->aiocb.aio_offset = 0;
> +    acb->aiocb.aio_flags = 0;
> +
> +    acb->next = posix_aio_state->first_aio;
> +    posix_aio_state->first_aio = acb;
> +
> +    acb->aiocb.aio_ioctl_buf = buf;
> +    acb->aiocb.aio_ioctl_cmd = req;
> +    if (qemu_paio_ioctl(&acb->aiocb) < 0) {
> +        raw_aio_remove(acb);
> +        return NULL;
> +    }
> +
> +    return &acb->common;
>  }
> +#endif

Shouldn't we just make these implementations totally unconditional
(bdrv_ioctl) or conditional on CONFIG_AIO only (bdrv_aio_ioctl)?

I can't think of any Posix-ish operating system that wouldn't provide
the ioctl system call.