Re: [Qemu-block] [PATCH 1/7] blockjobs: hide internal jobs from management API

[Kevin Wolf]

Am 14.10.2016 um 00:56 hat John Snow geschrieben:
> If jobs are not created directly by the user, do not allow them to be
> seen by the user/management utility. At the moment, 'internal' jobs are
> those that do not have an ID. As of this patch it is impossible to
> create such jobs.
> 
> Signed-off-by: John Snow 

block_job_get() still has a strcmp(id, job->id) for all block jobs
without checking job->id != NULL first.

job->id is also used in the error message in block_job_complete(),
though you could argue that we have a bug if this is ever triggered for
internal jobs. Still, there are platform on which this would crash, so
maybe better catch it.

> -BlockJobInfo *block_job_query(BlockJob *job)
> +BlockJobInfo *block_job_query(BlockJob *job, Error **errp)
>  {
> -BlockJobInfo *info = g_new0(BlockJobInfo, 1);
> +BlockJobInfo *info;
> +
> +if (block_job_is_internal(job)) {
> +error_setg(errp, "Cannot query QEMU internal Jobs");

You definitely have Potential for being a good Student of German. I
agree that a Text is easier to read if you capitalise all Nouns in it,
but I'm afraid this is not Part of the current english Orthography.

> +return NULL;
> +}
> +info = g_new0(BlockJobInfo, 1);
>  info->type  = g_strdup(BlockJobType_lookup[job->driver->job_type]);
>  info->device= g_strdup(job->id);
>  info->len   = job->len;

Kevin

[Qemu-block] [PATCH 1/7] blockjobs: hide internal jobs from management API

[John Snow]

If jobs are not created directly by the user, do not allow them to be
seen by the user/management utility. At the moment, 'internal' jobs are
those that do not have an ID. As of this patch it is impossible to
create such jobs.

Signed-off-by: John Snow 
---
 blockdev.c   | 17 +
 blockjob.c   | 37 +++--
 include/block/blockjob.h | 12 ++--
 3 files changed, 54 insertions(+), 12 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 07ec733..5904edb 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3915,13 +3915,22 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp)
 BlockJob *job;
 
 for (job = block_job_next(NULL); job; job = block_job_next(job)) {
-BlockJobInfoList *elem = g_new0(BlockJobInfoList, 1);
-AioContext *aio_context = blk_get_aio_context(job->blk);
+BlockJobInfoList *elem;
+AioContext *aio_context;
 
+if (block_job_is_internal(job)) {
+continue;
+}
+elem = g_new0(BlockJobInfoList, 1);
+aio_context = blk_get_aio_context(job->blk);
 aio_context_acquire(aio_context);
-elem->value = block_job_query(job);
+elem->value = block_job_query(job, errp);
 aio_context_release(aio_context);
-
+if (!elem->value) {
+g_free(elem);
+qapi_free_BlockJobInfoList(head);
+return NULL;
+}
 *p_next = elem;
 p_next = &elem->next;
 }
diff --git a/blockjob.c b/blockjob.c
index 43fecbe..e78ad94 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -185,6 +185,11 @@ void *block_job_create(const char *job_id, const 
BlockJobDriver *driver,
 return job;
 }
 
+bool block_job_is_internal(BlockJob *job)
+{
+return (job->id == NULL);
+}
+
 void block_job_ref(BlockJob *job)
 {
 ++job->refcnt;
@@ -494,9 +499,15 @@ void block_job_yield(BlockJob *job)
 block_job_pause_point(job);
 }
 
-BlockJobInfo *block_job_query(BlockJob *job)
+BlockJobInfo *block_job_query(BlockJob *job, Error **errp)
 {
-BlockJobInfo *info = g_new0(BlockJobInfo, 1);
+BlockJobInfo *info;
+
+if (block_job_is_internal(job)) {
+error_setg(errp, "Cannot query QEMU internal Jobs");
+return NULL;
+}
+info = g_new0(BlockJobInfo, 1);
 info->type  = g_strdup(BlockJobType_lookup[job->driver->job_type]);
 info->device= g_strdup(job->id);
 info->len   = job->len;
@@ -519,6 +530,10 @@ static void block_job_iostatus_set_err(BlockJob *job, int 
error)
 
 void block_job_event_cancelled(BlockJob *job)
 {
+if (block_job_is_internal(job)) {
+return;
+}
+
 qapi_event_send_block_job_cancelled(job->driver->job_type,
 job->id,
 job->len,
@@ -529,6 +544,10 @@ void block_job_event_cancelled(BlockJob *job)
 
 void block_job_event_completed(BlockJob *job, const char *msg)
 {
+if (block_job_is_internal(job)) {
+return;
+}
+
 qapi_event_send_block_job_completed(job->driver->job_type,
 job->id,
 job->len,
@@ -543,6 +562,10 @@ void block_job_event_ready(BlockJob *job)
 {
 job->ready = true;
 
+if (block_job_is_internal(job)) {
+return;
+}
+
 qapi_event_send_block_job_ready(job->driver->job_type,
 job->id,
 job->len,
@@ -573,10 +596,12 @@ BlockErrorAction block_job_error_action(BlockJob *job, 
BlockdevOnError on_err,
 default:
 abort();
 }
-qapi_event_send_block_job_error(job->id,
-is_read ? IO_OPERATION_TYPE_READ :
-IO_OPERATION_TYPE_WRITE,
-action, &error_abort);
+if (!block_job_is_internal(job)) {
+qapi_event_send_block_job_error(job->id,
+is_read ? IO_OPERATION_TYPE_READ :
+IO_OPERATION_TYPE_WRITE,
+action, &error_abort);
+}
 if (action == BLOCK_ERROR_ACTION_STOP) {
 /* make the pause user visible, which will be resumed from QMP. */
 job->user_paused = true;
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 4ddb4ae..6ecfa2e 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -107,7 +107,7 @@ struct BlockJob {
 BlockBackend *blk;
 
 /**
- * The ID of the block job.
+ * The ID of the block job. May be NULL for internal jobs.
  */
 char *id;
 
@@ -333,7 +333,7 @@ bool block_job_is_cancelled(BlockJob *job);
  *
  * Return information about a job.
  */
-BlockJobInfo *block_job_query(BlockJob *job);
+BlockJobInfo *block_job_query(BlockJob *job, Error **errp);
 
 /**
  * block_job_pause_point:
@@ -504,4 +504,12 @@ void block_job_txn_unref