Re: [Qemu-devel] [PATCH 2/2] block: Convert bdrv_first to QTAILQ

[Stefan Hajnoczi <stefanha@gmail.com>]

On Fri, Apr 9, 2010 at 5:47 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> @@ -545,13 +542,15 @@ void bdrv_close(BlockDriverState *bs)
>>
>>  void bdrv_delete(BlockDriverState *bs)
>>  {
>> -    BlockDriverState **pbs;
>> +    BlockDriverState *bs1;
>>
>> -    pbs = &bdrv_first;
>> -    while (*pbs != bs && *pbs != NULL)
>> -        pbs = &(*pbs)->next;
>> -    if (*pbs == bs)
>> -        *pbs = bs->next;
>> +    /* remove from list, if necessary */
>> +    QTAILQ_FOREACH(bs1, &bdrv_states, list) {
>> +        if (bs1 == bs) {
>> +            QTAILQ_REMOVE(&bdrv_states, bs, list);
>> +            break;
>> +        }
>> +    }
>
> This loop looks strange, what is it used for? We had only a next pointer
> previously, so we needed to search the element. A QTAILQ has both prev
> and next pointers though, so you should be able to directly use
> QTAILQ_REMOVE.

Only named BlockDriverStates are added to the tail queue.  Those with
an empty string as their name will not be on the tail queue.

The QTAILQ_REMOVE macro will not work if the element isn't on the tail
queue.  I didn't see a clean way to check if an element is on a tail
queue using qemu-queue.h, so I kept the search behavior.  The check I
want is tge_prev != NULL, I think.

I see three options:
1. Leave the search.
2. Modify qemu-queue.h to add a QTAILQ_ON_LIST(elm) macro.
3. Break the QTAILQ abstraction and test tge_prev directly.

What do you think?

Stefan