| Message ID | 1343127865-16608-5-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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. *
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(-)