Re: [Qemu-devel] [PATCH v2 08/23] block: Eliminate BlockDriverState member device_name[]

[Markus Armbruster]

Benoît Canet <address@hidden> writes:

>>  #include "qemu-common.h"
>> -#include "block/block_int.h"
>> +#include "block/block.h"
>> +#include "qemu/error-report.h"
>> +#include "qemu/main-loop.h"
>
> That's a lot of include fiddling I am not sure to understand them while 
> looking
> at the following diff.

Before the patch, we need block_int.h because we dereference bs.

After the patch, we don't, so I can drop block_int.h.  But then I have
to add back those includes included by block_int.h we actually need
here.

>>  #include "hw/hw.h"
>>  #include "qemu/queue.h"
>>  #include "qemu/timer.h"
>> @@ -130,9 +132,9 @@ static void blk_send(QEMUFile *f, BlkMigBlock * blk)
>>                       | flags);
>>  
>>      /* device name */
>> -    len = strlen(blk->bmds->bs->device_name);
>> +    len = strlen(bdrv_get_device_name(blk->bmds->bs));
>>      qemu_put_byte(f, len);
>> -    qemu_put_buffer(f, (uint8_t *)blk->bmds->bs->device_name, len);
>> +    qemu_put_buffer(f, (uint8_t *)bdrv_get_device_name(blk->bmds->bs), len);
>>  
>>      /* if a block is zero we need to flush here since the network
>>       * bandwidth is now a lot higher than the storage device bandwidth.
>> @@ -382,9 +384,9 @@ static void init_blk_migration(QEMUFile *f)
>>  
>>          if (bmds->shared_base) {
>>              DPRINTF("Start migration for %s with shared base image\n",
>> -                    bs->device_name);
>> +                    bdrv_get_device_name(bs));
>>          } else {
>> -            DPRINTF("Start full migration for %s\n", bs->device_name);
>> +            DPRINTF("Start full migration for %s\n", 
>> bdrv_get_device_name(bs));
>>          }
>>  
>>          QSIMPLEQ_INSERT_TAIL(&block_mig_state.bmds_list, bmds, entry);
>
>
>> @@ -1959,10 +1941,10 @@ void bdrv_drain_all(void)
>>   V  Also, NULL terminate the device_name to prevent double remove */
>>  void bdrv_make_anon(BlockDriverState *bs)
>>  {
>> -    if (bs->device_name[0] != '\0') {
>
>
>> +    if (bs->device_list.tqe_prev) {
>>          QTAILQ_REMOVE(&bdrv_states, bs, device_list);
>> +        bs->device_list.tqe_prev = NULL;
>
> I think a comments explaining the trick you are doing here would be worthy:
> after all you are touching directly the inner parts of a linked list and
> bypassing the list API.

Fair enough.

>>      }
>> -    bs->device_name[0] = '\0';
>>      if (bs->node_name[0] != '\0') {
>>          QTAILQ_REMOVE(&graph_bdrv_states, bs, node_list);
>>      }