Hi Konrad,

first of all, thank you for your review. You noticed quite a few points
I never really looked at because I inherited them from the current
tapdisk code. But probably I should fix these issues as well. ;-)

Konrad Rzeszutek schrieb:
>> +	blkif->fds[READ] = open_ctrl_socket(wrctldev);
>> +	blkif->fds[WRITE] = open_ctrl_socket(rdctldev);
> 
> How about freeing the data here once?
> 
>> +	
>> +	if (blkif->fds[READ] == -1 || blkif->fds[WRITE] == -1) {
>> +		free(rdctldev);
>> +		free(wrctldev);
> 
> And then this is not needed.
> 
>> +		return -1;
>> +	}
>> +
>> +	DPRINTF("Attached to qemu blktap pipes\n");
>> +	free(rdctldev);
>> +	free(wrctldev);
> 
> Nor these two lines above.

Hmm, good point. This code looks a bit silly... Will move the free to
the place you suggested.

>> --- a/tools/python/xen/xend/server/BlktapController.py	Mon Mar 10 22:51:57 2008 +0000
>> +++ b/tools/python/xen/xend/server/BlktapController.py	Thu Mar 13 13:00:18 2008 +0100
>> @@ -13,7 +13,9 @@ blktap_disk_types = [
>>      'vmdk',
>>      'ram',
>>      'qcow',
>> -    'qcow2'
>> +    'qcow2',
>> +
>> +    '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.

>> +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.

>> +			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.

I've attached a new version of the patch.

Kevin