Patchwork [04/47] block: add block_job_query

login
register
mail settings
Submitter Paolo Bonzini
Date July 24, 2012, 11:03 a.m.
Message ID <1343127865-16608-5-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/172840/
State New
Headers show

Comments

Paolo Bonzini - July 24, 2012, 11:03 a.m.
Extract it out of the implementation of query-block-jobs.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 blockdev.c |   15 ++-------------
 blockjob.c |   11 +++++++++++
 blockjob.h |    8 ++++++++
 3 files changed, 21 insertions(+), 13 deletions(-)
Kevin Wolf - July 30, 2012, 2:47 p.m.
Am 24.07.2012 13:03, schrieb Paolo Bonzini:
> Extract it out of the implementation of query-block-jobs.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  blockdev.c |   15 ++-------------
>  blockjob.c |   11 +++++++++++
>  blockjob.h |    8 ++++++++
>  3 files changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index e066f8f..dc099f9 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1166,19 +1166,8 @@ static void do_qmp_query_block_jobs_one(void *opaque, BlockDriverState *bs)
>      BlockJob *job = bs->job;
>  
>      if (job) {
> -        BlockJobInfoList *elem;
> -        BlockJobInfo *info = g_new(BlockJobInfo, 1);
> -        *info = (BlockJobInfo){
> -            .type   = g_strdup(job->job_type->job_type),
> -            .device = g_strdup(bdrv_get_device_name(bs)),
> -            .len    = job->len,
> -            .offset = job->offset,
> -            .speed  = job->speed,
> -        };
> -
> -        elem = g_new0(BlockJobInfoList, 1);
> -        elem->value = info;
> -
> +        BlockJobInfoList *elem = g_new0(BlockJobInfoList, 1);
> +        elem->value = block_job_query(bs->job);
>          (*prev)->next = elem;
>          *prev = elem;
>      }
> diff --git a/blockjob.c b/blockjob.c
> index 9737a43..a947a6e 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -161,3 +161,14 @@ void block_job_sleep_ns(BlockJob *job, QEMUClock *clock, int64_t ns)
>          job->busy = true;
>      }
>  }
> +
> +BlockJobInfo *block_job_query(BlockJob *job)
> +{
> +    BlockJobInfo *info = g_new(BlockJobInfo, 1);
> +    info->type   = g_strdup(job->job_type->job_type);
> +    info->device = g_strdup(bdrv_get_device_name(job->bs));
> +    info->len    = job->len;
> +    info->offset = job->offset;
> +    info->speed  = job->speed;
> +    return info;
> +}

Why did you convert the initialisation to separate statement? If you
really want to do this, I think using g_new0 would be safer now, but I
actually like compound literals better.

Kevin
Paolo Bonzini - July 30, 2012, 3:05 p.m.
Il 30/07/2012 16:47, Kevin Wolf ha scritto:
>> > +BlockJobInfo *block_job_query(BlockJob *job)
>> > +{
>> > +    BlockJobInfo *info = g_new(BlockJobInfo, 1);
>> > +    info->type   = g_strdup(job->job_type->job_type);
>> > +    info->device = g_strdup(bdrv_get_device_name(job->bs));
>> > +    info->len    = job->len;
>> > +    info->offset = job->offset;
>> > +    info->speed  = job->speed;
>> > +    return info;
>> > +}
> Why did you convert the initialisation to separate statement? If you
> really want to do this, I think using g_new0 would be safer now, but I
> actually like compound literals better.

Later on I will have some more initialization beyond the list of fields,
so I preferred an explicit list.  I can change it back if you prefer.

Paolo
Kevin Wolf - July 31, 2012, 8:47 a.m.
Am 30.07.2012 17:05, schrieb Paolo Bonzini:
> Il 30/07/2012 16:47, Kevin Wolf ha scritto:
>>>> +BlockJobInfo *block_job_query(BlockJob *job)
>>>> +{
>>>> +    BlockJobInfo *info = g_new(BlockJobInfo, 1);
>>>> +    info->type   = g_strdup(job->job_type->job_type);
>>>> +    info->device = g_strdup(bdrv_get_device_name(job->bs));
>>>> +    info->len    = job->len;
>>>> +    info->offset = job->offset;
>>>> +    info->speed  = job->speed;
>>>> +    return info;
>>>> +}
>> Why did you convert the initialisation to separate statement? If you
>> really want to do this, I think using g_new0 would be safer now, but I
>> actually like compound literals better.
> 
> Later on I will have some more initialization beyond the list of fields,
> so I preferred an explicit list.  I can change it back if you prefer.

What I'm really interested in is having zero-initialisation for any not
explicitly initialised fields, just to be on the safe side. You can do
that with g_new0() or with compound literals, that's a matter of taste.
My taste happens to prefer the latter, but I won't criticise a patch
based on taste as long as it's doing the same thing functionally.

Kevin
Paolo Bonzini - July 31, 2012, 8:50 a.m.
Il 31/07/2012 10:47, Kevin Wolf ha scritto:
>>> >> Why did you convert the initialisation to separate statement? If you
>>> >> really want to do this, I think using g_new0 would be safer now, but I
>>> >> actually like compound literals better.
>> > 
>> > Later on I will have some more initialization beyond the list of fields,
>> > so I preferred an explicit list.  I can change it back if you prefer.
> What I'm really interested in is having zero-initialisation for any not
> explicitly initialised fields, just to be on the safe side. You can do
> that with g_new0() or with compound literals, that's a matter of taste.

Yes, and in fact I even have a change to g_new0 later in the series.
I'll squash that change in this patch.

Paolo
Jeff Cody - Aug. 2, 2012, 7:28 p.m.
On 07/31/2012 04:50 AM, Paolo Bonzini wrote:
> Il 31/07/2012 10:47, Kevin Wolf ha scritto:
>>>>>> Why did you convert the initialisation to separate statement? If you
>>>>>> really want to do this, I think using g_new0 would be safer now, but I
>>>>>> actually like compound literals better.
>>>>
>>>> Later on I will have some more initialization beyond the list of fields,
>>>> so I preferred an explicit list.  I can change it back if you prefer.
>> What I'm really interested in is having zero-initialisation for any not
>> explicitly initialised fields, just to be on the safe side. You can do
>> that with g_new0() or with compound literals, that's a matter of taste.
> 
> Yes, and in fact I even have a change to g_new0 later in the series.
> I'll squash that change in this patch.
> 
> Paolo
> 
> 

+1 on this...  interestingly, I just ran into an issue with this patch
while testing block-job-query on my commit patches - I got a segfault on
the command query, because has_target was not initialized to 0, and so
the target ptr was invalid.  Changing it to g_new0() fixes it.


Jeff

Patch

diff --git a/blockdev.c b/blockdev.c
index e066f8f..dc099f9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1166,19 +1166,8 @@  static void do_qmp_query_block_jobs_one(void *opaque, BlockDriverState *bs)
     BlockJob *job = bs->job;
 
     if (job) {
-        BlockJobInfoList *elem;
-        BlockJobInfo *info = g_new(BlockJobInfo, 1);
-        *info = (BlockJobInfo){
-            .type   = g_strdup(job->job_type->job_type),
-            .device = g_strdup(bdrv_get_device_name(bs)),
-            .len    = job->len,
-            .offset = job->offset,
-            .speed  = job->speed,
-        };
-
-        elem = g_new0(BlockJobInfoList, 1);
-        elem->value = info;
-
+        BlockJobInfoList *elem = g_new0(BlockJobInfoList, 1);
+        elem->value = block_job_query(bs->job);
         (*prev)->next = elem;
         *prev = elem;
     }
diff --git a/blockjob.c b/blockjob.c
index 9737a43..a947a6e 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -161,3 +161,14 @@  void block_job_sleep_ns(BlockJob *job, QEMUClock *clock, int64_t ns)
         job->busy = true;
     }
 }
+
+BlockJobInfo *block_job_query(BlockJob *job)
+{
+    BlockJobInfo *info = g_new(BlockJobInfo, 1);
+    info->type   = g_strdup(job->job_type->job_type);
+    info->device = g_strdup(bdrv_get_device_name(job->bs));
+    info->len    = job->len;
+    info->offset = job->offset;
+    info->speed  = job->speed;
+    return info;
+}
diff --git a/blockjob.h b/blockjob.h
index 1b4c7be..347a62f 100644
--- a/blockjob.h
+++ b/blockjob.h
@@ -163,6 +163,14 @@  void block_job_cancel(BlockJob *job);
 bool block_job_is_cancelled(BlockJob *job);
 
 /**
+ * block_job_query:
+ * @job: The job to get information about.
+ *
+ * Return information about a job.
+ */
+BlockJobInfo *block_job_query(BlockJob *job);
+
+/**
  * block_job_cancel_sync:
  * @job: The job to be canceled.
  *