Re: [Xen-devel] [RFC][PATCH] Use ioemu block drivers through blktap

[Konrad Rzeszutek <konrad@xxxxxxxxxxxxxxx>]

On Fri, Mar 14, 2008 at 10:37:48AM +0100, Kevin Wolf wrote:
> Hi Konrad,
> 
> first of all, thank you for your review. You noticed quite a few points

You are welcome.

> I never really looked at because I inherited them from the current
> tapdisk code. But probably I should fix these issues as well. ;-)

Yes :-)


.. snip ..
> >> +
> >> +    'ioemu'
> > 
> > Why add the extra \n ?
> 
> I wanted to separate the ioemu pseudo driver (which is the only one that
> doesn't go through tapdisk) from the "real" tapdisk drivers.

OK. How about you add a comment saying exactly what you just said
in that line?

> 
> >> +static struct td_state *state_init(void)
> >> +{
> >> +  int i;
> >> +  struct td_state *s;
> >> +  blkif_t *blkif;
> >> +
> >> +  s = malloc(sizeof(struct td_state));
> > 
> > Would it make sense to zero out the allocated memory?
> 
> This code comes directly from tapdisk and it worked there. On the other
> hand, it certainly wouldn't hurt.

I am thinking that in the future the struct td_state might be expanded and
not initializing all of the variables to something could lead to corrupt
pointers.

> 
> >> +                  switch (req->operation) 
> >> +                  {
> >> +                  case BLKIF_OP_WRITE:
> >> +                          aiocb_info = malloc(sizeof(*aiocb_info));
> >> +
> >> +                          aiocb_info->s = s;
> >> +                          aiocb_info->sector = sector_nr;
> >> +                          aiocb_info->nr_secs = nsects;
> >> +                          aiocb_info->idx = idx;
> >> +                          aiocb_info->i = i;
> >> +
> >> +                          ret = (NULL == bdrv_aio_write(s->bs, sector_nr,
> >> +                                                    page, nsects,
> >> +                                                    qemu_send_responses,
> >> +                                                    aiocb_info));
> > 
> > Who de-allocates aiocb_info?
> 
> qemu_send_responses is a callback function which gets aiocb_info as
> parameter and frees it when it's done.

Aha! I thought so but wasn't sure. Would it make sense to include
your explanation as comment?

> 
> I've attached a new version of the patch.

Thanks. Didn't spot anything wrong.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel