Re: [PATCH v6 38/42] nvme: support multiple namespaces

[Klaus Birkelund Jensen]

On Mar 25 12:59, Maxim Levitsky wrote:
> On Mon, 2020-03-16 at 07:29 -0700, Klaus Jensen wrote:
> > From: Klaus Jensen 
> > 
> > This adds support for multiple namespaces by introducing a new 'nvme-ns'
> > device model. The nvme device creates a bus named from the device name
> > ('id'). The nvme-ns devices then connect to this and registers
> > themselves with the nvme device.
> > 
> > This changes how an nvme device is created. Example with two namespaces:
> > 
> >   -drive file=nvme0n1.img,if=none,id=disk1
> >   -drive file=nvme0n2.img,if=none,id=disk2
> >   -device nvme,serial=deadbeef,id=nvme0
> >   -device nvme-ns,drive=disk1,bus=nvme0,nsid=1
> >   -device nvme-ns,drive=disk2,bus=nvme0,nsid=2
> > 
> > The drive property is kept on the nvme device to keep the change
> > backward compatible, but the property is now optional. Specifying a
> > drive for the nvme device will always create the namespace with nsid 1.
> > 
> > Signed-off-by: Klaus Jensen 
> > Signed-off-by: Klaus Jensen 
> > Reviewed-by: Keith Busch 
> > ---
> >  hw/block/Makefile.objs |   2 +-
> >  hw/block/nvme-ns.c | 157 +++
> >  hw/block/nvme-ns.h |  60 +++
> >  hw/block/nvme.c| 233 ++---
> >  hw/block/nvme.h|  47 -
> >  hw/block/trace-events  |   4 +-
> >  6 files changed, 389 insertions(+), 114 deletions(-)
> >  create mode 100644 hw/block/nvme-ns.c
> >  create mode 100644 hw/block/nvme-ns.h
> > 
> > diff --git a/hw/block/Makefile.objs b/hw/block/Makefile.objs
> > index 4b4a2b338dc4..d9141d6a4b9b 100644
> > --- a/hw/block/Makefile.objs
> > +++ b/hw/block/Makefile.objs

> > @@ -2518,9 +2561,6 @@ static void nvme_init_ctrl(NvmeCtrl *n)
> >  id->psd[0].mp = cpu_to_le16(0x9c4);
> >  id->psd[0].enlat = cpu_to_le32(0x10);
> >  id->psd[0].exlat = cpu_to_le32(0x4);
> > -if (blk_enable_write_cache(n->conf.blk)) {
> > -id->vwc = 1;
> > -}
> Shouldn't that be kept? Assuming that user used the legacy 'drive' option,
> and it had no write cache enabled.
> 

When using the drive option we still end up calling the same code that
handles the "new style" namespaces and that code will handle the write
cache similary.

> >  
> >  n->bar.cap = 0;
> >  NVME_CAP_SET_MQES(n->bar.cap, 0x7ff);
> > @@ -2533,25 +2573,34 @@ static void nvme_init_ctrl(NvmeCtrl *n)
> >  n->bar.intmc = n->bar.intms = 0;
> >  }
> >  
> > -static int nvme_init_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error 
> > **errp)
> > +int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
> >  {
> > -int64_t bs_size;
> > -NvmeIdNs *id_ns = >id_ns;
> > +uint32_t nsid = nvme_nsid(ns);
> >  
> > -bs_size = blk_getlength(n->conf.blk);
> > -if (bs_size < 0) {
> > -error_setg_errno(errp, -bs_size, "blk_getlength");
> > +if (nsid > NVME_MAX_NAMESPACES) {
> > +error_setg(errp, "invalid nsid (must be between 0 and %d)",
> > +   NVME_MAX_NAMESPACES);
> >  return -1;
> >  }
> >  
> > -id_ns->lbaf[0].ds = BDRV_SECTOR_BITS;
> > -n->ns_size = bs_size;
> > +if (!nsid) {
> > +for (int i = 1; i <= n->num_namespaces; i++) {
> > +NvmeNamespace *ns = nvme_ns(n, i);
> > +if (!ns) {
> > +nsid = i;
> > +break;
> > +}
> > +}
> This misses an edge error case, where all the namespaces are allocated.
> Yes, it would be insane to allocate all 256 namespaces but still.
> 

Impressive catch! Fixed!

> 
> > +} else {
> > +if (n->namespaces[nsid - 1]) {
> > +error_setg(errp, "nsid must be unique");
> 
> I''l would change that error message to something like 
> "namespace id %d is already in use" or something like that.
> 

Done.

Re: [PATCH v6 32/42] nvme: allow multiple aios per command

[Klaus Birkelund Jensen]

On Mar 25 12:57, Maxim Levitsky wrote:
> On Mon, 2020-03-16 at 07:29 -0700, Klaus Jensen wrote:
> > From: Klaus Jensen 
> > 
> > This refactors how the device issues asynchronous block backend
> > requests. The NvmeRequest now holds a queue of NvmeAIOs that are
> > associated with the command. This allows multiple aios to be issued for
> > a command. Only when all requests have been completed will the device
> > post a completion queue entry.
> > 
> > Because the device is currently guaranteed to only issue a single aio
> > request per command, the benefit is not immediately obvious. But this
> > functionality is required to support metadata, the dataset management
> > command and other features.
> > 
> > Signed-off-by: Klaus Jensen 
> > Signed-off-by: Klaus Jensen 
> > Acked-by: Keith Busch 
> > ---
> >  hw/block/nvme.c   | 377 +++---
> >  hw/block/nvme.h   | 129 +--
> >  hw/block/trace-events |   6 +
> >  3 files changed, 407 insertions(+), 105 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 0d2b5b45b0c5..817384e3b1a9 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -373,6 +374,99 @@ static uint16_t nvme_map(NvmeCtrl *n, NvmeCmd *cmd, 
> > QEMUSGList *qsg,
> >  return nvme_map_prp(n, qsg, iov, prp1, prp2, len, req);
> >  }
> >  
> > +static void nvme_aio_destroy(NvmeAIO *aio)
> > +{
> > +g_free(aio);
> > +}
> > +
> > +static inline void nvme_req_register_aio(NvmeRequest *req, NvmeAIO *aio,
> I guess I'll call this nvme_req_add_aio,
> or nvme_add_aio_to_reg.
> Thoughts?
> Also you can leave this as is, but add a comment on top explaining this
> 

nvme_req_add_aio it is :) And comment added.

> > + NvmeAIOOp opc)
> > +{
> > +aio->opc = opc;
> > +
> > +trace_nvme_dev_req_register_aio(nvme_cid(req), aio, blk_name(aio->blk),
> > +aio->offset, aio->len,
> > +nvme_aio_opc_str(aio), req);
> > +
> > +if (req) {
> > +QTAILQ_INSERT_TAIL(>aio_tailq, aio, tailq_entry);
> > +}
> > +}
> > +
> > +static void nvme_submit_aio(NvmeAIO *aio)
> OK, this name makes sense
> Also please add a comment on top.

Done.

> > @@ -505,9 +600,11 @@ static inline uint16_t nvme_check_mdts(NvmeCtrl *n, 
> > size_t len,
> >  return NVME_SUCCESS;
> >  }
> >  
> > -static inline uint16_t nvme_check_prinfo(NvmeCtrl *n, NvmeNamespace *ns,
> > - uint16_t ctrl, NvmeRequest *req)
> > +static inline uint16_t nvme_check_prinfo(NvmeCtrl *n, uint16_t ctrl,
> > + NvmeRequest *req)
> >  {
> > +NvmeNamespace *ns = req->ns;
> > +
> This should go to the patch that added nvme_check_prinfo
> 

Probably killing that patch.

> > @@ -516,10 +613,10 @@ static inline uint16_t nvme_check_prinfo(NvmeCtrl *n, 
> > NvmeNamespace *ns,
> >  return NVME_SUCCESS;
> >  }
> >  
> > -static inline uint16_t nvme_check_bounds(NvmeCtrl *n, NvmeNamespace *ns,
> > - uint64_t slba, uint32_t nlb,
> > - NvmeRequest *req)
> > +static inline uint16_t nvme_check_bounds(NvmeCtrl *n, uint64_t slba,
> > + uint32_t nlb, NvmeRequest *req)
> >  {
> > +NvmeNamespace *ns = req->ns;
> >  uint64_t nsze = le64_to_cpu(ns->id_ns.nsze);
> This should go to the patch that added nvme_check_bounds as well
> 

We can't really, because the NvmeRequest does not hold a reference to
the namespace as a struct member at that point. This is also an issue
with the nvme_check_prinfo function above.

> >  
> >  if (unlikely(UINT64_MAX - slba < nlb || slba + nlb > nsze)) {
> > @@ -530,55 +627,154 @@ static inline uint16_t nvme_check_bounds(NvmeCtrl 
> > *n, NvmeNamespace *ns,
> >  return NVME_SUCCESS;
> >  }
> >  
> > -static void nvme_rw_cb(void *opaque, int ret)
> > +static uint16_t nvme_check_rw(NvmeCtrl *n, NvmeRequest *req)
> > +{
> > +NvmeNamespace *ns = req->ns;
> > +NvmeRwCmd *rw = (NvmeRwCmd *) >cmd;
> > +uint16_t ctrl = le16_to_cpu(rw->control);
> > +size_t len = req->nlb << nvme_ns_lbads(ns);
> > +uint16_t status;
> > +
> > +status = nvme_check_mdts(n, len, req);
> > +if (status) {
> > +return status;
> > +}
> > +
> > +status = nvme_check_prinfo(n, ctrl, req);
> > +if (status) {
> > +return status;
> > +}
> > +
> > +status = nvme_check_bounds(n, req->slba, req->nlb, req);
> > +if (status) {
> > +return status;
> > +}
> > +
> > +return NVME_SUCCESS;
> > +}
> 
> Nitpick: I hate to say it but nvme_check_rw should be in a separate patch as 
> well.
> It will also make diff more readable (when adding a funtion and changing a 
> function
> at the same time, you get a diff between two unrelated things)
> 

Done, but had to do it as a follow up patch.

> >  

Re: [PATCH v6 36/42] nvme: add support for scatter gather lists

[Klaus Birkelund Jensen]

On Mar 25 12:58, Maxim Levitsky wrote:
> On Mon, 2020-03-16 at 07:29 -0700, Klaus Jensen wrote:
> > From: Klaus Jensen 
> > 
> > For now, support the Data Block, Segment and Last Segment descriptor
> > types.
> > 
> > See NVM Express 1.3d, Section 4.4 ("Scatter Gather List (SGL)").
> > 
> > Signed-off-by: Klaus Jensen 
> > Acked-by: Keith Busch 
> > ---
> >  hw/block/nvme.c   | 310 +++---
> >  hw/block/trace-events |   4 +
> >  2 files changed, 262 insertions(+), 52 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 49d323566393..b89b96990f52 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -76,7 +76,12 @@ static inline bool nvme_addr_is_cmb(NvmeCtrl *n, hwaddr 
> > addr)
> >  
> >  static int nvme_addr_read(NvmeCtrl *n, hwaddr addr, void *buf, int size)
> >  {
> > -if (n->bar.cmbsz && nvme_addr_is_cmb(n, addr)) {
> > +hwaddr hi = addr + size;
> > +if (hi < addr) {
> > +return 1;
> > +}
> > +
> > +if (n->bar.cmbsz && nvme_addr_is_cmb(n, addr) && nvme_addr_is_cmb(n, 
> > hi)) {
> 
> I would suggest to split this into a separate patch as well, since this 
> contains not just one but 2 bugfixes
> for this function and they are not related to sg lists.
> Or at least move this to 'nvme: refactor nvme_addr_read' and rename this patch
> to something like 'nvme: fix and refactor nvme_addr_read'
> 

I've split it into a patch.

> 
> >  memcpy(buf, nvme_addr_to_cmb(n, addr), size);
> >  return 0;
> >  }
> > @@ -328,13 +333,242 @@ unmap:
> >  return status;
> >  }
> >  
> > -static uint16_t nvme_dma_prp(NvmeCtrl *n, uint8_t *ptr, uint32_t len,
> > - uint64_t prp1, uint64_t prp2, DMADirection 
> > dir,
> > +static uint16_t nvme_map_sgl_data(NvmeCtrl *n, QEMUSGList *qsg,
> > +  QEMUIOVector *iov,
> > +  NvmeSglDescriptor *segment, uint64_t 
> > nsgld,
> > +  size_t *len, NvmeRequest *req)
> > +{
> > +dma_addr_t addr, trans_len;
> > +uint32_t blk_len;
> > +uint16_t status;
> > +
> > +for (int i = 0; i < nsgld; i++) {
> > +uint8_t type = NVME_SGL_TYPE(segment[i].type);
> > +
> > +if (type != NVME_SGL_DESCR_TYPE_DATA_BLOCK) {
> > +switch (type) {
> > +case NVME_SGL_DESCR_TYPE_BIT_BUCKET:
> > +case NVME_SGL_DESCR_TYPE_KEYED_DATA_BLOCK:
> > +return NVME_SGL_DESCR_TYPE_INVALID | NVME_DNR;
> > +default:
> To be honest I don't like that 'default'
> I would explicitly state which segment types remain 
> (I think segment list and last segment list, and various reserved types)
> In fact for the reserved types you probably also want to return 
> NVME_SGL_DESCR_TYPE_INVALID)
> 

I "negated" the logic which I think is more readable. I still really
want to keep the default, for instance, nvme v1.4 adds a new type that
we do not support (the Transport SGL Data Block descriptor).

> Also this function as well really begs to have a description prior to it,
> something like 'map a sg list section, assuming that it only contains SGL 
> data descriptions,
> caller has to ensure this'.
> 

Done.

> 
> > +return NVME_INVALID_NUM_SGL_DESCRS | NVME_DNR;
> > +}
> > +}
> > +
> > +if (*len == 0) {
> > +uint16_t sgls = le16_to_cpu(n->id_ctrl.sgls);
> Nitpick: I would add a small comment here as well describiing
> what this does (We reach this point if sg list covers more that that
> was specified in the commmand, and the NVME_CTRL_SGLS_EXCESS_LENGTH controller
> capability indicates that we support just throwing the extra data away)
> 

Adding a comment. It's the other way around. The size as indicated by
NLB (or whatever depending on the command) is the "authoritative" souce
of information for the size of the payload. We will never accept an SGL
that is too short such that we lose or throw away data, but we might
accept ignoring parts of the SGL.

> > +if (sgls & NVME_CTRL_SGLS_EXCESS_LENGTH) {
> > +break;
> > +}
> > +
> > +trace_nvme_dev_err_invalid_sgl_excess_length(nvme_cid(req));
> > +return NVME_DATA_SGL_LEN_INVALID | NVME_DNR;
> > +}
> > +
> > +addr = le64_to_cpu(segment[i].addr);
> > +blk_len = le32_to_cpu(segment[i].len);
> > +
> > +if (!blk_len) {
> > +continue;
> > +}
> > +
> > +if (UINT64_MAX - addr < blk_len) {
> > +return NVME_DATA_SGL_LEN_INVALID | NVME_DNR;
> > +}
> Good!
> > +
> > +trans_len = MIN(*len, blk_len);
> > +
> > +status = nvme_map_addr(n, qsg, iov, addr, trans_len);
> > +if (status) {
> > +return status;
> > +}
> > +
> > +*len -= trans_len;
> > +}
> > +
> > +return NVME_SUCCESS;
> > +}
> > +
> > +static uint16_t 

Re: [PATCH v6 35/42] nvme: handle dma errors

[Klaus Birkelund Jensen]

On Mar 25 12:58, Maxim Levitsky wrote:
> On Mon, 2020-03-16 at 07:29 -0700, Klaus Jensen wrote:
> > From: Klaus Jensen 
> > 
> > Handling DMA errors gracefully is required for the device to pass the
> > block/011 test ("disable PCI device while doing I/O") in the blktests
> > suite.
> > 
> > With this patch the device passes the test by retrying "critical"
> > transfers (posting of completion entries and processing of submission
> > queue entries).
> > 
> > If DMA errors occur at any other point in the execution of the command
> > (say, while mapping the PRPs), the command is aborted with a Data
> > Transfer Error status code.
> > 
> > Signed-off-by: Klaus Jensen 
> > Acked-by: Keith Busch 
> > ---
> >  hw/block/nvme.c   | 45 ---
> >  hw/block/trace-events |  2 ++
> >  include/block/nvme.h  |  2 +-
> >  3 files changed, 37 insertions(+), 12 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 15ca2417af04..49d323566393 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -164,7 +164,7 @@ static uint16_t nvme_map_addr_cmb(NvmeCtrl *n, 
> > QEMUIOVector *iov, hwaddr addr,
> >size_t len)
> >  {
> >  if (!nvme_addr_is_cmb(n, addr) || !nvme_addr_is_cmb(n, addr + len)) {
> > -return NVME_DATA_TRAS_ERROR;
> > +return NVME_DATA_TRANSFER_ERROR;
> 
> Minor nitpick: this is also a non functional refactoring.
> I don't think that each piece of a refactoring should be in a separate patch,
> so I usually group all the non functional (aka cosmetic) refactoring in one 
> patch, usually the first in the series.
> But I try not to leave such refactoring in the functional patches.
> 
> However, since there is not that much cases like that left, I don't mind 
> leaving this particular case as is.
> 

Noted. Keeping it here for now ;)

> 
> Reviewed-by: Maxim Levitsky 
> 
> Best regards,
>   Maxim Levitsky
> 

Re: [PATCH v6 31/42] nvme: add check for prinfo

[Klaus Birkelund Jensen]

On Mar 25 12:57, Maxim Levitsky wrote:
> On Mon, 2020-03-16 at 07:29 -0700, Klaus Jensen wrote:
> > From: Klaus Jensen 
> > 
> > Check the validity of the PRINFO field.
> > 
> > Signed-off-by: Klaus Jensen 
> > ---
> >  hw/block/nvme.c   | 50 ---
> >  hw/block/trace-events |  1 +
> >  include/block/nvme.h  |  1 +
> >  3 files changed, 44 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 7d5340c272c6..0d2b5b45b0c5 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -505,6 +505,17 @@ static inline uint16_t nvme_check_mdts(NvmeCtrl *n, 
> > size_t len,
> >  return NVME_SUCCESS;
> >  }
> >  
> > +static inline uint16_t nvme_check_prinfo(NvmeCtrl *n, NvmeNamespace *ns,
> > + uint16_t ctrl, NvmeRequest *req)
> > +{
> > +if ((ctrl & NVME_RW_PRINFO_PRACT) && !(ns->id_ns.dps & DPS_TYPE_MASK)) 
> > {
> > +trace_nvme_dev_err_prinfo(nvme_cid(req), ctrl);
> > +return NVME_INVALID_FIELD | NVME_DNR;
> > +}
> 
> I refreshed my (still very limited) knowelege on the metadata
> and the protection info, and this is what I found:
> 
> I think that this is very far from complete, because we also have:
> 
> 1. PRCHECK. According to the spec it is independent of PRACT
>And when some of it is set, 
>together with enabled protection (the DPS field in namespace),
>Then the 8 bytes of the protection info is checked (optionally using the
>the EILBRT and ELBAT/ELBATM fields in the command and CRC of the data for 
> the guard field)
> 
>So this field should also be checked to be zero when protection is disabled
>(I don't see an explicit requirement for that in the spec, but neither I 
> see
>such requirement for PRACT)
> 
> 2. The protection values to be written / checked ((E)ILBRT/(E)LBATM/(E)LBAT)
>Same here, but also these should not be set when PRCHECK is not set for 
> reads,
>plus some are protection type specific.
> 
> 
> The spec does mention the 'Invalid Protection Information' error code which
> refers to invalid values in the PRINFO field.
> So this error code I think should be returned instead of the 'Invalid field'
> 
> Another thing to optionaly check is that the metadata pointer for separate 
> metadata.
>  Is zero as long as we don't support metadata
> (again I don't see an explicit requirement for this in the spec, but it 
> mentions:
> 
> "This field is valid only if the command has metadata that is not interleaved 
> with
> the logical block data, as specified in the Format NVM command"
> 
> )
> 

I'm kinda inclined to just drop this patch. The spec actually says that
the PRACT and PRCHK fields are used only if the namespace is formatted
to use end-to-end protection information. Since we do not support that,
I don't think we even need to check it.

Any opinion on this?

Re: [PATCH v6 23/42] nvme: add mapping helpers

[Klaus Birkelund Jensen]

On Mar 25 12:45, Maxim Levitsky wrote:
> On Mon, 2020-03-16 at 07:29 -0700, Klaus Jensen wrote:
> > From: Klaus Jensen 
> > 
> > Add nvme_map_addr, nvme_map_addr_cmb and nvme_addr_to_cmb helpers and
> > use them in nvme_map_prp.
> > 
> > This fixes a bug where in the case of a CMB transfer, the device would
> > map to the buffer with a wrong length.
> > 
> > Fixes: b2b2b67a00574 ("nvme: Add support for Read Data and Write Data in 
> > CMBs.")
> > Signed-off-by: Klaus Jensen 
> > ---
> >  hw/block/nvme.c   | 97 +++
> >  hw/block/trace-events |  1 +
> >  2 files changed, 81 insertions(+), 17 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 08267e847671..187c816eb6ad 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -153,29 +158,79 @@ static void nvme_irq_deassert(NvmeCtrl *n, NvmeCQueue 
> > *cq)
> >  }
> >  }
> >  
> > +static uint16_t nvme_map_addr_cmb(NvmeCtrl *n, QEMUIOVector *iov, hwaddr 
> > addr,
> > +  size_t len)
> > +{
> > +if (!nvme_addr_is_cmb(n, addr) || !nvme_addr_is_cmb(n, addr + len)) {
> > +return NVME_DATA_TRAS_ERROR;
> > +}
> 
> I just noticed that
> in theory (not that it really matters) but addr+len refers to the byte which 
> is already 
> not the part of the transfer.
> 

Oh. Good catch - and I think that it does matter? Can't we end up
rejecting a valid access? Anyway, I fixed it with a '- 1'.

> 
> > +
> > +qemu_iovec_add(iov, nvme_addr_to_cmb(n, addr), len);
> Also intersting is we can add 0 sized iovec.
> 

I added a check on len. This also makes sure the above '- 1' fix doesn't
cause an 'addr + 0 - 1' to be done.

Re: [PATCH v6 24/42] nvme: remove redundant has_sg member

[Klaus Birkelund Jensen]

On Mar 25 12:45, Maxim Levitsky wrote:
> On Mon, 2020-03-16 at 07:29 -0700, Klaus Jensen wrote:
> > From: Klaus Jensen 
> > 
> > Remove the has_sg member from NvmeRequest since it's redundant.
> 
> To be honest this patch also replaces the dma_acct_start with block_acct_start
> which looks right to me, and IMHO its OK to have both in the same patch,
> but that should be mentioned in the commit message
> 

I pulled it to a separate patch :)

> With this fixed,
> Reviewed-by: Maxim Levitsky 
> 

Re: [PATCH v6 29/42] nvme: refactor request bounds checking

[Klaus Birkelund Jensen]

On Mar 25 12:56, Maxim Levitsky wrote:
> On Mon, 2020-03-16 at 07:29 -0700, Klaus Jensen wrote:
> > From: Klaus Jensen 
> > 
> > Signed-off-by: Klaus Jensen 
> > ---
> >  hw/block/nvme.c | 28 ++--
> >  1 file changed, 22 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index eecfad694bf8..ba520c76bae5 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -562,13 +577,14 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeNamespace 
> > *ns, NvmeCmd *cmd,
> >  uint64_t data_offset = slba << data_shift;
> >  int is_write = rw->opcode == NVME_CMD_WRITE ? 1 : 0;
> >  enum BlockAcctType acct = is_write ? BLOCK_ACCT_WRITE : 
> > BLOCK_ACCT_READ;
> > +uint16_t status;
> >  
> >  trace_nvme_dev_rw(is_write ? "write" : "read", nlb, data_size, slba);
> >  
> > -if (unlikely((slba + nlb) > ns->id_ns.nsze)) {
> > +status = nvme_check_bounds(n, ns, slba, nlb, req);
> > +if (status) {
> >  block_acct_invalid(blk_get_stats(n->conf.blk), acct);
> > -trace_nvme_dev_err_invalid_lba_range(slba, nlb, ns->id_ns.nsze);
> > -return NVME_LBA_RANGE | NVME_DNR;
> > +return status;
> >  }
> >  
> >  if (nvme_map(n, cmd, >qsg, >iov, data_size, req)) {
> Looks good as well, once we get support for discard, it will
> use this as well, but for now indeed only write zeros and read/write
> need bounds checking on the IO path.
> 

I have that patch in the submission queue and the check is factored out
there ;)

> Reviewed-by: Maxim Levitsky 
> 

Re: [PATCH v6 14/42] nvme: add missing mandatory features

[Klaus Birkelund Jensen]

On Mar 25 12:41, Maxim Levitsky wrote:
> On Mon, 2020-03-16 at 07:29 -0700, Klaus Jensen wrote:
> > From: Klaus Jensen 
> > 
> > Add support for returning a resonable response to Get/Set Features of
> > mandatory features.
> > 
> > Signed-off-by: Klaus Jensen 
> > Acked-by: Keith Busch 
> > ---
> >  hw/block/nvme.c   | 60 ++-
> >  hw/block/trace-events |  2 ++
> >  include/block/nvme.h  |  6 -
> >  3 files changed, 66 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index ff8975cd6667..eb9c722df968 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -1058,6 +1069,19 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, 
> > NvmeCmd *cmd, NvmeRequest *req)
> >  break;
> >  case NVME_TIMESTAMP:
> >  return nvme_get_feature_timestamp(n, cmd);
> > +case NVME_INTERRUPT_COALESCING:
> > +result = cpu_to_le32(n->features.int_coalescing);
> > +break;
> > +case NVME_INTERRUPT_VECTOR_CONF:
> > +if ((dw11 & 0x) > n->params.max_ioqpairs + 1) {
> > +return NVME_INVALID_FIELD | NVME_DNR;
> > +}
> I still think that this should be >= since the interrupt vector is not zero 
> based.
> So if we have for example 3 IO queues, then we have 4 queues in total
> which translates to irq numbers 0..3.
> 

Yes you are right. The device will support max_ioqpairs + 1 IVs, so
trying to access that would actually go 1 beyond the array.

Fixed.

> BTW the user of the device doesn't have to have 1:1 mapping between qid and 
> msi interrupt index,
> in fact when MSI is not used, all the queues will map to the same vector, 
> which will be interrupt 0
> from point of view of the device IMHO.
> So it kind of makes sense IMHO to have num_irqs or something, even if it 
> technically equals to number of queues.
> 

Yeah, but the device will still *support* the N IVs, so they can still
be configured even though they will not be used. So I don't think we
need to introduce an additional parameter?

> > @@ -1120,6 +1146,10 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, 
> > NvmeCmd *cmd, NvmeRequest *req)
> >  
> >  break;
> >  case NVME_VOLATILE_WRITE_CACHE:
> > +if (blk_enable_write_cache(n->conf.blk)) {
> > +blk_flush(n->conf.blk);
> > +}
> 
> (not your fault) but the blk_enable_write_cache function name is highly 
> misleading,
> since it doesn't enable anything but just gets the flag if the write cache is 
> enabled.
> It really should be called blk_get_enable_write_cache.
> 

Agreed :)

> > @@ -1804,6 +1860,7 @@ static void nvme_init_ctrl(NvmeCtrl *n)
> >  id->cqes = (0x4 << 4) | 0x4;
> >  id->nn = cpu_to_le32(n->num_namespaces);
> >  id->oncs = cpu_to_le16(NVME_ONCS_WRITE_ZEROS | NVME_ONCS_TIMESTAMP);
> > +
> Unrelated whitespace change

Fixed.

> 
> Best regards,
>   Maxim Levitsky
> 
> 
> 
> 

Re: [PATCH v6 12/42] nvme: add support for the get log page command

[Klaus Birkelund Jensen]

On Mar 25 12:40, Maxim Levitsky wrote:
> On Mon, 2020-03-16 at 07:28 -0700, Klaus Jensen wrote:
> > From: Klaus Jensen 
> > 
> > Add support for the Get Log Page command and basic implementations of
> > the mandatory Error Information, SMART / Health Information and Firmware
> > Slot Information log pages.
> > 
> > In violation of the specification, the SMART / Health Information log
> > page does not persist information over the lifetime of the controller
> > because the device has no place to store such persistent state.
> > 
> > Note that the LPA field in the Identify Controller data structure
> > intentionally has bit 0 cleared because there is no namespace specific
> > information in the SMART / Health information log page.
> > 
> > Required for compliance with NVMe revision 1.2.1. See NVM Express 1.2.1,
> > Section 5.10 ("Get Log Page command").
> > 
> > Signed-off-by: Klaus Jensen 
> > Acked-by: Keith Busch 
> > ---
> >  hw/block/nvme.c   | 138 +-
> >  hw/block/nvme.h   |  10 +++
> >  hw/block/trace-events |   2 +
> >  3 files changed, 149 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 64c42101df5c..83ff3fbfb463 100644
> >
> > +static uint16_t nvme_error_info(NvmeCtrl *n, NvmeCmd *cmd, uint32_t 
> > buf_len,
> > +uint64_t off, NvmeRequest *req)
> > +{
> > +uint32_t trans_len;
> > +uint64_t prp1 = le64_to_cpu(cmd->dptr.prp1);
> > +uint64_t prp2 = le64_to_cpu(cmd->dptr.prp2);
> > +uint8_t errlog[64];
> I'll would replace this with sizeof(NvmeErrorLogEntry)
> (and add NvmeErrorLogEntry to the nvme.h), just for the sake of consistency,
> and in case we end up reporting some errors to the log in the future.
> 

NvmeErrorLog is already in nvme.h; Fixed to actually use it.

> 
> > +
> > +if (off > sizeof(errlog)) {
> > +return NVME_INVALID_FIELD | NVME_DNR;
> > +}
> > +
> > +memset(errlog, 0x0, sizeof(errlog));
> > +
> > +trans_len = MIN(sizeof(errlog) - off, buf_len);
> > +
> > +return nvme_dma_read_prp(n, errlog, trans_len, prp1, prp2);
> > +}
> Besides this, looks good now.
> 
> 
> Best regards,
>   Maxim Levitsky
> 

Re: [PATCH v6 19/42] nvme: enforce valid queue creation sequence

[Klaus Birkelund Jensen]

On Mar 25 12:43, Maxim Levitsky wrote:
> On Mon, 2020-03-16 at 07:29 -0700, Klaus Jensen wrote:
> > From: Klaus Jensen 
> > 
> > Support returning Command Sequence Error if Set Features on Number of
> > Queues is called after queues have been created.
> > 
> > Signed-off-by: Klaus Jensen 
> > ---
> >  hw/block/nvme.c | 7 +++
> >  hw/block/nvme.h | 1 +
> >  2 files changed, 8 insertions(+)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 007f8817f101..b40d27cddc46 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -881,6 +881,8 @@ static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeCmd 
> > *cmd)
> >  cq = g_malloc0(sizeof(*cq));
> >  nvme_init_cq(cq, n, prp1, cqid, vector, qsize + 1,
> >  NVME_CQ_FLAGS_IEN(qflags));
> > +
> > +n->qs_created = true;
> Very minor nitpick, maybe it is worth mentioning in a comment,
> why this is only needed in CQ creation, as you explained to me.
> 

Added.

> 
> Reviewed-by: Maxim Levitsky 
> 
> Best regards,
>   Maxim Levitsky
> 
> 
> 
> 
> 

Re: [PATCH v6 11/42] nvme: add temperature threshold feature

[Klaus Birkelund Jensen]

On Mar 25 12:40, Maxim Levitsky wrote:
> On Mon, 2020-03-16 at 07:28 -0700, Klaus Jensen wrote:
> > From: Klaus Jensen 
> > 
> > It might seem wierd to implement this feature for an emulated device,
> > but it is mandatory to support and the feature is useful for testing
> > asynchronous event request support, which will be added in a later
> > patch.
> > 
> > Signed-off-by: Klaus Jensen 
> > Acked-by: Keith Busch 
> > ---
> >  hw/block/nvme.c  | 48 
> >  hw/block/nvme.h  |  2 ++
> >  include/block/nvme.h |  8 +++-
> >  3 files changed, 57 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> > index b7c465560eea..8cda5f02c622 100644
> > --- a/hw/block/nvme.h
> > +++ b/hw/block/nvme.h
> > @@ -108,6 +108,7 @@ typedef struct NvmeCtrl {
> >  uint64_tirq_status;
> >  uint64_thost_timestamp; /* Timestamp sent by the 
> > host */
> >  uint64_ttimestamp_set_qemu_clock_ms;/* QEMU clock time */
> > +uint16_ttemperature;
> You forgot to move this too.
> 

Fixed!
> 
> With 'temperature' field removed from the header:
> 
> Reviewed-by: Maxim Levitsky 
> 
> Best regards,
>   Maxim Levitsky
> 

Re: [PATCH v6 10/42] nvme: refactor device realization

[Klaus Birkelund Jensen]

On Mar 25 12:40, Maxim Levitsky wrote:
> On Mon, 2020-03-16 at 07:28 -0700, Klaus Jensen wrote:
> > From: Klaus Jensen 
> > 
> > This patch splits up nvme_realize into multiple individual functions,
> > each initializing a different subset of the device.
> > 
> > Signed-off-by: Klaus Jensen 
> > Acked-by: Keith Busch 
> > ---
> >  hw/block/nvme.c | 178 ++--
> >  hw/block/nvme.h |  23 ++-
> >  2 files changed, 134 insertions(+), 67 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 7dfd8a1a392d..665485045066 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -1340,57 +1337,100 @@ static void nvme_realize(PCIDevice *pci_dev, Error 
> > **errp)
> >  n->params.max_ioqpairs = n->params.num_queues - 1;
> >  }
> >  
> > -if (!n->params.max_ioqpairs) {
> > -error_setg(errp, "max_ioqpairs can't be less than 1");
> > +if (params->max_ioqpairs < 1 ||
> > +params->max_ioqpairs > PCI_MSIX_FLAGS_QSIZE) {
> > +error_setg(errp, "nvme: max_ioqpairs must be ");
> Looks like the error message is not complete now.

Fixed!

> 
> Small nitpick: To be honest this not only refactoring in the device 
> realization since you also (rightfully)
> removed the duplicated cmbsz/cmbloc so I would add a mention for this in the 
> commit message.
> But that doesn't matter that much, so
> 

You are right. I've added it as a separate patch.

> Reviewed-by: Maxim Levitsky 
> 
> Best regards,
>   Maxim Levitsky
> 

Re: [PATCH v6 09/42] nvme: add max_ioqpairs device parameter

[Klaus Birkelund Jensen]

On Mar 25 12:39, Maxim Levitsky wrote:
> On Mon, 2020-03-16 at 07:28 -0700, Klaus Jensen wrote:
> > From: Klaus Jensen 
> > 
> > The num_queues device paramater has a slightly confusing meaning because
> > it accounts for the admin queue pair which is not really optional.
> > Secondly, it is really a maximum value of queues allowed.
> > 
> > Add a new max_ioqpairs parameter that only accounts for I/O queue pairs,
> > but keep num_queues for compatibility.
> > 
> > Signed-off-by: Klaus Jensen 
> > ---
> >  hw/block/nvme.c | 45 ++---
> >  hw/block/nvme.h |  4 +++-
> >  2 files changed, 29 insertions(+), 20 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 7cf7cf55143e..7dfd8a1a392d 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -1332,9 +1333,15 @@ static void nvme_realize(PCIDevice *pci_dev, Error 
> > **errp)
> >  int64_t bs_size;
> >  uint8_t *pci_conf;
> >  
> > -if (!n->params.num_queues) {
> > -error_setg(errp, "num_queues can't be zero");
> > -return;
> > +if (n->params.num_queues) {
> > +warn_report("nvme: num_queues is deprecated; please use 
> > max_ioqpairs "
> > +"instead");
> > +
> > +n->params.max_ioqpairs = n->params.num_queues - 1;
> > +}
> > +
> > +if (!n->params.max_ioqpairs) {
> > +error_setg(errp, "max_ioqpairs can't be less than 1");
> >  }
> This is not even a nitpick, but just and idea.
> 
> It might be worth it to allow max_ioqpairs=0 to simulate a 'broken'
> nvme controller. I know that kernel has special handling for such controllers,
> which include only creation of the control character device (/dev/nvme*) 
> through
> which the user can submit commands to try and 'fix' the controller (by 
> re-uploading firmware
> maybe or something like that).
> 
> 

Not sure about the implications of this, so I'll leave that on the TODO
:) But a controller with no I/O queues is an "Administrative Controller"
and perfectly legal in NVMe v1.4 AFAIK.

> >  
> >  if (!n->conf.blk) {
> > @@ -1365,19 +1372,19 @@ static void nvme_realize(PCIDevice *pci_dev, Error 
> > **errp)
> >  pcie_endpoint_cap_init(pci_dev, 0x80);
> >  
> >  n->num_namespaces = 1;
> > -n->reg_size = pow2ceil(0x1004 + 2 * (n->params.num_queues + 1) * 4);
> > +n->reg_size = pow2ceil(0x1008 + 2 * (n->params.max_ioqpairs) * 4);
> 
> I hate to say it, but it looks like this thing (which I mentioned to you in 
> V5)
> was pre-existing bug, which is indeed fixed now.
> In theory such fixes should go to separate patches, but in this case, I guess 
> it would
> be too much to ask for it.
> Maybe mention this in the commit message instead, so that this fix doesn't 
> stay hidden like that?
> 
> 

I'm convinced now. I have added a preparatory bugfix patch before this
patch.

> 
> Reviewed-by: Maxim Levitsky 
> 
> Best regards,
>   Maxim Levitsky
> 

Re: [PATCH v6 05/42] nvme: use constant for identify data size

[Klaus Birkelund Jensen]

On Mar 25 12:37, Maxim Levitsky wrote:
> On Mon, 2020-03-16 at 07:28 -0700, Klaus Jensen wrote:
> > From: Klaus Jensen 
> > 
> > Signed-off-by: Klaus Jensen 
> > ---
> >  hw/block/nvme.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 40cb176dea3c..f716f690a594 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -679,7 +679,7 @@ static uint16_t nvme_identify_ns(NvmeCtrl *n, 
> > NvmeIdentify *c)
> >  
> >  static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c)
> >  {
> > -static const int data_len = 4 * KiB;
> > +static const int data_len = NVME_IDENTIFY_DATA_SIZE;
> >  uint32_t min_nsid = le32_to_cpu(c->nsid);
> >  uint64_t prp1 = le64_to_cpu(c->prp1);
> >  uint64_t prp2 = le64_to_cpu(c->prp2);
> 
> I'll probably squash this with some other refactoring patch,
> but I absolutely don't mind leaving this as is.
> Fine grained patches never cause any harm.
> 

Squashed into 06/42.

> Reviewed-by: Maxim Levitsky 
> 
> Best regards,
>   Maxim Levitsky
> 
> 
> 
> 

Re: [PATCH v6 06/42] nvme: add identify cns values in header

[Klaus Birkelund Jensen]

On Mar 25 12:37, Maxim Levitsky wrote:
> On Mon, 2020-03-16 at 07:28 -0700, Klaus Jensen wrote:
> > From: Klaus Jensen 
> > 
> > Signed-off-by: Klaus Jensen 
> > ---
> >  hw/block/nvme.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index f716f690a594..b38d7e548a60 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -709,11 +709,11 @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd 
> > *cmd)
> >  NvmeIdentify *c = (NvmeIdentify *)cmd;
> >  
> >  switch (le32_to_cpu(c->cns)) {
> > -case 0x00:
> > +case NVME_ID_CNS_NS:
> >  return nvme_identify_ns(n, c);
> > -case 0x01:
> > +case NVME_ID_CNS_CTRL:
> >  return nvme_identify_ctrl(n, c);
> > -case 0x02:
> > +case NVME_ID_CNS_NS_ACTIVE_LIST:
> >  return nvme_identify_nslist(n, c);
> >  default:
> >  trace_nvme_dev_err_invalid_identify_cns(le32_to_cpu(c->cns));
> 
> This is a very good candidate to be squished with the patch 5 IMHO,
> but you can leave this as is as well. I don't mind.
> 

Squashed!

> Reviewed-by: Maxim Levitsky 
> 
> Best regards,
>   Maxim Levitsky
> 
> 
> 
> 
> 
> 

Re: [PATCH v6 07/42] nvme: refactor nvme_addr_read

[Klaus Birkelund Jensen]

On Mar 25 12:38, Maxim Levitsky wrote:
> On Mon, 2020-03-16 at 07:28 -0700, Klaus Jensen wrote:
> > From: Klaus Jensen 
> > 
> > Pull the controller memory buffer check to its own function. The check
> > will be used on its own in later patches.
> > 
> > Signed-off-by: Klaus Jensen 
> > Acked-by: Keith Busch 
> > ---
> >  hw/block/nvme.c | 16 
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index b38d7e548a60..08a83d449de3 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -52,14 +52,22 @@
> >  
> >  static void nvme_process_sq(void *opaque);
> >  
> > +static inline bool nvme_addr_is_cmb(NvmeCtrl *n, hwaddr addr)
> > +{
> > +hwaddr low = n->ctrl_mem.addr;
> > +hwaddr hi  = n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size);
> > +
> > +return addr >= low && addr < hi;
> > +}
> > +
> >  static void nvme_addr_read(NvmeCtrl *n, hwaddr addr, void *buf, int size)
> >  {
> > -if (n->cmbsz && addr >= n->ctrl_mem.addr &&
> > -addr < (n->ctrl_mem.addr + 
> > int128_get64(n->ctrl_mem.size))) {
> > +if (n->cmbsz && nvme_addr_is_cmb(n, addr)) {
> >  memcpy(buf, (void *)>cmbuf[addr - n->ctrl_mem.addr], size);
> > -} else {
> > -pci_dma_read(>parent_obj, addr, buf, size);
> > +return;
> >  }
> > +
> > +pci_dma_read(>parent_obj, addr, buf, size);
> >  }
> >  
> >  static int nvme_check_sqid(NvmeCtrl *n, uint16_t sqid)
> 
> Note that this patch still contains a bug that it removes the check against 
> the accessed
> size, which you fix in later patch.
> I prefer to not add a bug in first place
> However if you have a reason for this, I won't mind.
> 

So yeah. The resons is that there is actually no bug at this point
because the controller only supports PRPs. I actually thought there was
a bug as well and reported it to qemu-security some months ago as a
potential out of bounds access. I was then schooled by Keith on how PRPs
work ;) Below is a paraphrased version of Keiths analysis.

The PRPs does not cross page boundaries:

trans_len = n->page_size - (prp1 % n->page_size);

The PRPs are always verified to be page aligned:

if (unlikely(!prp_ent || prp_ent & (n->page_size - 1))) {

and the transfer length wont go above page size. So, since the beginning
of the address is within the CMB and considering that the CMB is of an
MB aligned and sized granularity, then we can never cross outside it
with PRPs.

I could add the check at this point (because it *is* needed for when
SGLs are introduced), but I think it would just be noise and I would
need to explain why the check is there, but not really needed at this
point. Instead I'm adding a new patch before the SGL patch that explains
this.

Re: [PATCH v6 04/42] nvme: bump spec data structures to v1.3

[Klaus Birkelund Jensen]

On Mar 25 12:37, Maxim Levitsky wrote:
> On Mon, 2020-03-16 at 07:28 -0700, Klaus Jensen wrote:
> > From: Klaus Jensen 
> > 
> > Add missing fields in the Identify Controller and Identify Namespace
> > data structures to bring them in line with NVMe v1.3.
> > 
> > This also adds data structures and defines for SGL support which
> > requires a couple of trivial changes to the nvme block driver as well.
> > 
> > Signed-off-by: Klaus Jensen 
> > Acked-by: Fam Zheng 
> > ---
> >  block/nvme.c |  18 ++---
> >  hw/block/nvme.c  |  12 ++--
> >  include/block/nvme.h | 153 ++-
> >  3 files changed, 151 insertions(+), 32 deletions(-)
> > 
> > diff --git a/block/nvme.c b/block/nvme.c
> > index d41c4bda6e39..99b9bb3dac96 100644
> > --- a/block/nvme.c
> > +++ b/block/nvme.c
> > @@ -589,6 +675,16 @@ enum NvmeIdCtrlOncs {
> >  #define NVME_CTRL_CQES_MIN(cqes) ((cqes) & 0xf)
> >  #define NVME_CTRL_CQES_MAX(cqes) (((cqes) >> 4) & 0xf)
> >  
> > +#define NVME_CTRL_SGLS_SUPPORTED_MASK(0x3 <<  0)
> > +#define NVME_CTRL_SGLS_SUPPORTED_NO_ALIGNMENT(0x1 <<  0)
> > +#define NVME_CTRL_SGLS_SUPPORTED_DWORD_ALIGNMENT (0x1 <<  1)
> > +#define NVME_CTRL_SGLS_KEYED (0x1 <<  2)
> > +#define NVME_CTRL_SGLS_BITBUCKET (0x1 << 16)
> > +#define NVME_CTRL_SGLS_MPTR_CONTIGUOUS   (0x1 << 17)
> > +#define NVME_CTRL_SGLS_EXCESS_LENGTH (0x1 << 18)
> > +#define NVME_CTRL_SGLS_MPTR_SGL  (0x1 << 19)
> > +#define NVME_CTRL_SGLS_ADDR_OFFSET   (0x1 << 20)
> OK
> > +
> >  typedef struct NvmeFeatureVal {
> >  uint32_tarbitration;
> >  uint32_tpower_mgmt;
> > @@ -611,6 +707,10 @@ typedef struct NvmeFeatureVal {
> >  #define NVME_INTC_THR(intc) (intc & 0xff)
> >  #define NVME_INTC_TIME(intc)((intc >> 8) & 0xff)
> >  
> > +#define NVME_TEMP_THSEL(temp)  ((temp >> 20) & 0x3)
> Nitpick: If we are adding this, I'll add a #define for the values as well
> 

Done. And used in the subsequent "nvme: add temperature threshold
feature" patch.

> > +#define NVME_TEMP_TMPSEL(temp) ((temp >> 16) & 0xf)
> > +#define NVME_TEMP_TMPTH(temp)  ((temp >>  0) & 0x)
> > +
> >  enum NvmeFeatureIds {
> >  NVME_ARBITRATION= 0x1,
> >  NVME_POWER_MANAGEMENT   = 0x2,
> > @@ -653,18 +753,37 @@ typedef struct NvmeIdNs {
> >  uint8_t mc;
> >  uint8_t dpc;
> >  uint8_t dps;
> > -
> >  uint8_t nmic;
> >  uint8_t rescap;
> >  uint8_t fpi;
> >  uint8_t dlfeat;
> > -
> > -uint8_t res34[94];
> > +uint16_tnawun;
> > +uint16_tnawupf;
> > +uint16_tnacwu;
> > +uint16_tnabsn;
> > +uint16_tnabo;
> > +uint16_tnabspf;
> > +uint16_tnoiob;
> > +uint8_t nvmcap[16];
> > +uint8_t rsvd64[40];
> > +uint8_t nguid[16];
> > +uint64_teui64;
> >  NvmeLBAFlbaf[16];
> > -uint8_t res192[192];
> > +uint8_t rsvd192[192];
> >  uint8_t vs[3712];
> >  } NvmeIdNs;
> Also checked this against V5, looks OK now
> 
> >  
> > +typedef struct NvmeIdNsDescr {
> > +uint8_t nidt;
> > +uint8_t nidl;
> > +uint8_t rsvd2[2];
> > +} NvmeIdNsDescr;
> OK
> 
> 
> 
> > +
> > +#define NVME_NIDT_UUID_LEN 16
> > +
> > +enum {
> > +NVME_NIDT_UUID = 0x3,
> Very minor nitpick: I'll would add others as well just for the sake
> of better understanding what this is
> 

Done.

> > +};
> >  
> >  /*Deallocate Logical Block Features*/
> >  #define NVME_ID_NS_DLFEAT_GUARD_CRC(dlfeat)   ((dlfeat) & 0x10)
> 
> Looks very good.
> 
> Reviewed-by: Maxim Levitsky 
> 
> Best regards,
>   Maxim Levitsky
> 

Re: [PATCH v6 01/42] nvme: rename trace events to nvme_dev

[Klaus Birkelund Jensen]

On Mar 25 12:36, Maxim Levitsky wrote:
> On Mon, 2020-03-16 at 07:28 -0700, Klaus Jensen wrote:
> > From: Klaus Jensen 
> > 
> > Change the prefix of all nvme device related trace events to 'nvme_dev'
> > to not clash with trace events from the nvme block driver.
> > 
> > Signed-off-by: Klaus Jensen 
> > Acked-by: Keith Busch 
> > Reviewed-by: Maxim Levitsky 
> > ---
> >  hw/block/nvme.c   | 188 +-
> >  hw/block/trace-events | 172 +++---
> >  2 files changed, 180 insertions(+), 180 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index d28335cbf377..3e4b18956ed2 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -1035,32 +1035,32 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr 
> > offset, uint64_t data,
> >  switch (offset) {
> >  case 0xc:   /* INTMS */
> >  if (unlikely(msix_enabled(&(n->parent_obj {
> > -NVME_GUEST_ERR(nvme_ub_mmiowr_intmask_with_msix,
> > +NVME_GUEST_ERR(nvme_dev_ub_mmiowr_intmask_with_msix,
> > "undefined access to interrupt mask set"
> > " when MSI-X is enabled");
> >  /* should be ignored, fall through for now */
> >  }
> >  n->bar.intms |= data & 0x;
> >  n->bar.intmc = n->bar.intms;
> > -trace_nvme_mmio_intm_set(data & 0x,
> > +trace_nvme_dev_mmio_intm_set(data & 0x,
> >   n->bar.intmc);
> Indention.
> 

Fixed.

> >  nvme_irq_check(n);
> >  break;
> >  case 0x10:  /* INTMC */
> >  if (unlikely(msix_enabled(&(n->parent_obj {
> > -NVME_GUEST_ERR(nvme_ub_mmiowr_intmask_with_msix,
> > +NVME_GUEST_ERR(nvme_dev_ub_mmiowr_intmask_with_msix,
> > "undefined access to interrupt mask clr"
> > " when MSI-X is enabled");
> >  /* should be ignored, fall through for now */
> >  }
> >  n->bar.intms &= ~(data & 0x);
> >  n->bar.intmc = n->bar.intms;
> > -trace_nvme_mmio_intm_clr(data & 0x,
> > +trace_nvme_dev_mmio_intm_clr(data & 0x,
> >   n->bar.intmc);
> Indention.
> 

Fixed.

> 
> 
> Other that indention nitpicks, no changes vs V5,
> so my reviewed-by kept correctly.
> 
> Best regards,
>   Maxim Levitsky
> 

[PATCH v10 13/14] iotests: Mark verify functions as private

[John Snow]

Mark the verify functions as "private" with a leading underscore, to
discourage their use. Update type signatures while we're here.

(Also, make pending patches not yet using the new entry points fail in a
very obvious way.)

Signed-off-by: John Snow 
---
 tests/qemu-iotests/iotests.py | 23 +--
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 21f96b35e4..fd8cb36622 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -1005,7 +1005,8 @@ def case_notrun(reason):
 open('%s/%s.casenotrun' % (output_dir, seq), 'a').write(
 '[case not run] ' + reason + '\n')
 
-def verify_image_format(supported_fmts=(), unsupported_fmts=()):
+def _verify_image_format(supported_fmts: Sequence[str] = (),
+ unsupported_fmts: Sequence[str] = ()) -> None:
 assert not (supported_fmts and unsupported_fmts)
 
 if 'generic' in supported_fmts and \
@@ -1019,7 +1020,8 @@ def verify_image_format(supported_fmts=(), 
unsupported_fmts=()):
 if not_sup or (imgfmt in unsupported_fmts):
 notrun('not suitable for this image format: %s' % imgfmt)
 
-def verify_protocol(supported=(), unsupported=()):
+def _verify_protocol(supported: Sequence[str] = (),
+ unsupported: Sequence[str] = ()) -> None:
 assert not (supported and unsupported)
 
 if 'generic' in supported:
@@ -1029,7 +1031,8 @@ def verify_protocol(supported=(), unsupported=()):
 if not_sup or (imgproto in unsupported):
 notrun('not suitable for this protocol: %s' % imgproto)
 
-def verify_platform(supported=(), unsupported=()):
+def _verify_platform(supported: Sequence[str] = (),
+ unsupported: Sequence[str] = ()) -> None:
 if any((sys.platform.startswith(x) for x in unsupported)):
 notrun('not suitable for this OS: %s' % sys.platform)
 
@@ -1037,11 +1040,11 @@ def verify_platform(supported=(), unsupported=()):
 if not any((sys.platform.startswith(x) for x in supported)):
 notrun('not suitable for this OS: %s' % sys.platform)
 
-def verify_cache_mode(supported_cache_modes=()):
+def _verify_cache_mode(supported_cache_modes: Sequence[str] = ()) -> None:
 if supported_cache_modes and (cachemode not in supported_cache_modes):
 notrun('not suitable for this cache mode: %s' % cachemode)
 
-def verify_aio_mode(supported_aio_modes=()):
+def _verify_aio_mode(supported_aio_modes: Sequence[str] = ()):
 if supported_aio_modes and (aiomode not in supported_aio_modes):
 notrun('not suitable for this aio mode: %s' % aiomode)
 
@@ -1169,11 +1172,11 @@ def execute_setup_common(supported_fmts: Sequence[str] 
= (),
 sys.stderr.write('Please run this test via the "check" script\n')
 sys.exit(os.EX_USAGE)
 
-verify_image_format(supported_fmts, unsupported_fmts)
-verify_protocol(supported_protocols, unsupported_protocols)
-verify_platform(supported=supported_platforms)
-verify_cache_mode(supported_cache_modes)
-verify_aio_mode(supported_aio_modes)
+_verify_image_format(supported_fmts, unsupported_fmts)
+_verify_protocol(supported_protocols, unsupported_protocols)
+_verify_platform(supported=supported_platforms)
+_verify_cache_mode(supported_cache_modes)
+_verify_aio_mode(supported_aio_modes)
 
 debug = '-d' in sys.argv
 if debug:
-- 
2.21.1

[PATCH v10 11/14] iotests: add script_initialize

[John Snow]

Like script_main, but doesn't require a single point of entry.
Replace all existing initialization sections with this drop-in replacement.

This brings debug support to all existing script-style iotests.

Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/149|  3 +-
 tests/qemu-iotests/194|  4 +-
 tests/qemu-iotests/202|  4 +-
 tests/qemu-iotests/203|  4 +-
 tests/qemu-iotests/206|  2 +-
 tests/qemu-iotests/207|  6 ++-
 tests/qemu-iotests/208|  2 +-
 tests/qemu-iotests/209|  2 +-
 tests/qemu-iotests/210|  6 ++-
 tests/qemu-iotests/211|  6 ++-
 tests/qemu-iotests/212|  6 ++-
 tests/qemu-iotests/213|  6 ++-
 tests/qemu-iotests/216|  4 +-
 tests/qemu-iotests/218|  2 +-
 tests/qemu-iotests/219|  2 +-
 tests/qemu-iotests/222|  7 ++--
 tests/qemu-iotests/224|  4 +-
 tests/qemu-iotests/228|  6 ++-
 tests/qemu-iotests/234|  4 +-
 tests/qemu-iotests/235|  4 +-
 tests/qemu-iotests/236|  2 +-
 tests/qemu-iotests/237|  2 +-
 tests/qemu-iotests/238|  2 +
 tests/qemu-iotests/242|  2 +-
 tests/qemu-iotests/246|  2 +-
 tests/qemu-iotests/248|  2 +-
 tests/qemu-iotests/254|  2 +-
 tests/qemu-iotests/255|  2 +-
 tests/qemu-iotests/256|  2 +-
 tests/qemu-iotests/258|  7 ++--
 tests/qemu-iotests/260|  4 +-
 tests/qemu-iotests/262|  4 +-
 tests/qemu-iotests/264|  4 +-
 tests/qemu-iotests/277|  2 +
 tests/qemu-iotests/280|  8 ++--
 tests/qemu-iotests/283|  4 +-
 tests/qemu-iotests/iotests.py | 76 +++
 37 files changed, 130 insertions(+), 81 deletions(-)

diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
index b4a21bf7b7..852768f80a 100755
--- a/tests/qemu-iotests/149
+++ b/tests/qemu-iotests/149
@@ -382,8 +382,7 @@ def test_once(config, qemu_img=False):
 
 
 # Obviously we only work with the luks image format
-iotests.verify_image_format(supported_fmts=['luks'])
-iotests.verify_platform()
+iotests.script_initialize(supported_fmts=['luks'])
 
 # We need sudo in order to run cryptsetup to create
 # dm-crypt devices. This is safe to use on any
diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
index 9dc1bd3510..8b1f720af4 100755
--- a/tests/qemu-iotests/194
+++ b/tests/qemu-iotests/194
@@ -21,8 +21,8 @@
 
 import iotests
 
-iotests.verify_image_format(supported_fmts=['qcow2', 'qed', 'raw'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2', 'qed', 'raw'],
+  supported_platforms=['linux'])
 
 with iotests.FilePath('source.img') as source_img_path, \
  iotests.FilePath('dest.img') as dest_img_path, \
diff --git a/tests/qemu-iotests/202 b/tests/qemu-iotests/202
index 920a8683ef..e3900a44d1 100755
--- a/tests/qemu-iotests/202
+++ b/tests/qemu-iotests/202
@@ -24,8 +24,8 @@
 
 import iotests
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2'],
+  supported_platforms=['linux'])
 
 with iotests.FilePath('disk0.img') as disk0_img_path, \
  iotests.FilePath('disk1.img') as disk1_img_path, \
diff --git a/tests/qemu-iotests/203 b/tests/qemu-iotests/203
index 49eff5d405..4b4bd3307d 100755
--- a/tests/qemu-iotests/203
+++ b/tests/qemu-iotests/203
@@ -24,8 +24,8 @@
 
 import iotests
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2'],
+  supported_platforms=['linux'])
 
 with iotests.FilePath('disk0.img') as disk0_img_path, \
  iotests.FilePath('disk1.img') as disk1_img_path, \
diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206
index e2b50ae24d..f42432a838 100755
--- a/tests/qemu-iotests/206
+++ b/tests/qemu-iotests/206
@@ -23,7 +23,7 @@
 import iotests
 from iotests import imgfmt
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.script_initialize(supported_fmts=['qcow2'])
 
 with iotests.FilePath('t.qcow2') as disk_path, \
  iotests.FilePath('t.qcow2.base') as backing_path, \
diff --git a/tests/qemu-iotests/207 b/tests/qemu-iotests/207
index 3d9c1208ca..a6621410da 100755
--- a/tests/qemu-iotests/207
+++ b/tests/qemu-iotests/207
@@ -24,8 +24,10 @@ import iotests
 import subprocess
 import re
 
-iotests.verify_image_format(supported_fmts=['raw'])
-iotests.verify_protocol(supported=['ssh'])
+iotests.script_initialize(
+supported_fmts=['raw'],
+supported_protocols=['ssh'],
+)
 
 def filter_hash(qmsg):
 def _filter(key, value):
diff --git a/tests/qemu-iotests/208 b/tests/qemu-iotests/208
index 1c3fc8c7fd..6cb642f821 100755
--- a/tests/qemu-iotests/208
+++ b/tests/qemu-iotests/208
@@ -22,7 +22,7 @@
 
 import iotests
 

[PATCH v10 14/14] iotests: use python logging for iotests.log()

[John Snow]

We can turn logging on/off globally instead of per-function.

Remove use_log from run_job, and use python logging to turn on
diffable output when we run through a script entry point.

iotest 245 changes output order due to buffering reasons.


An extended note on python logging:

A NullHandler is added to `qemu.iotests` to stop output from being
generated if this code is used as a library without configuring logging.
A NullHandler is only needed at the root, so a duplicate handler is not
needed for `qemu.iotests.diff_io`.

When logging is not configured, messages at the 'WARNING' levels or
above are printed with default settings. The NullHandler stops this from
occurring, which is considered good hygiene for code used as a library.

See https://docs.python.org/3/howto/logging.html#library-config

When logging is actually enabled (always at the behest of an explicit
call by a client script), a root logger is implicitly created at the
root, which allows messages to propagate upwards and be handled/emitted
from the root logger with default settings.

When we want iotest logging, we attach a handler to the
qemu.iotests.diff_io logger and disable propagation to avoid possible
double-printing.

For more information on python logging infrastructure, I highly
recommend downloading the pip package `logging_tree`, which provides
convenient visualizations of the hierarchical logging configuration
under different circumstances.

See https://pypi.org/project/logging_tree/ for more information.

Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/030|  4 +--
 tests/qemu-iotests/155|  2 +-
 tests/qemu-iotests/245|  1 +
 tests/qemu-iotests/245.out| 24 
 tests/qemu-iotests/iotests.py | 53 ---
 5 files changed, 46 insertions(+), 38 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index aa911d266a..104e3cee1b 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -411,8 +411,8 @@ class TestParallelOps(iotests.QMPTestCase):
 result = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
 self.assert_qmp(result, 'return', {})
 
-self.vm.run_job(job='drive0', auto_dismiss=True, use_log=False)
-self.vm.run_job(job='node4', auto_dismiss=True, use_log=False)
+self.vm.run_job(job='drive0', auto_dismiss=True)
+self.vm.run_job(job='node4', auto_dismiss=True)
 self.assert_no_active_block_jobs()
 
 # Test a block-stream and a block-commit job in parallel
diff --git a/tests/qemu-iotests/155 b/tests/qemu-iotests/155
index 571bce9de4..cb371d4649 100755
--- a/tests/qemu-iotests/155
+++ b/tests/qemu-iotests/155
@@ -188,7 +188,7 @@ class MirrorBaseClass(BaseClass):
 
 self.assert_qmp(result, 'return', {})
 
-self.vm.run_job('mirror-job', use_log=False, auto_finalize=False,
+self.vm.run_job('mirror-job', auto_finalize=False,
 pre_finalize=self.openBacking, auto_dismiss=True)
 
 def testFull(self):
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index 1001275a44..4f5f0bb901 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -1027,5 +1027,6 @@ class TestBlockdevReopen(iotests.QMPTestCase):
 self.run_test_iothreads(None, 'iothread0')
 
 if __name__ == '__main__':
+iotests.activate_logging()
 iotests.main(supported_fmts=["qcow2"],
  supported_protocols=["file"])
diff --git a/tests/qemu-iotests/245.out b/tests/qemu-iotests/245.out
index 682b93394d..4b33dcaf5c 100644
--- a/tests/qemu-iotests/245.out
+++ b/tests/qemu-iotests/245.out
@@ -1,17 +1,17 @@
+{"execute": "job-finalize", "arguments": {"id": "commit0"}}
+{"return": {}}
+{"data": {"id": "commit0", "type": "commit"}, "event": "BLOCK_JOB_PENDING", 
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "commit0", "len": 3145728, "offset": 3145728, "speed": 0, 
"type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": 
{"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "job-finalize", "arguments": {"id": "stream0"}}
+{"return": {}}
+{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", 
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, 
"type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": 
{"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "job-finalize", "arguments": {"id": "stream0"}}
+{"return": {}}
+{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", 
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, 
"type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": 
{"microseconds": "USECS", "seconds": "SECS"}}
 .
 --
 

[PATCH v10 06/14] iotests: alphabetize standard imports

[John Snow]

I had to fix a merge conflict, so do this tiny harmless thing while I'm
here.

Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/iotests.py | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 51f84475d9..e6f9f62b2b 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -16,19 +16,19 @@
 # along with this program.  If not, see .
 #
 
+import atexit
+from collections import OrderedDict
+import faulthandler
+import io
+import json
+import logging
 import os
 import re
+import signal
+import struct
 import subprocess
-import unittest
 import sys
-import struct
-import json
-import signal
-import logging
-import atexit
-import io
-from collections import OrderedDict
-import faulthandler
+import unittest
 
 # pylint: disable=import-error, wrong-import-position
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-- 
2.21.1

[PATCH v10 07/14] iotests: drop pre-Python 3.4 compatibility code

[John Snow]

We no longer need to accommodate <3.4, drop this code.
(The lines were > 79 chars and it stood out.)

Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/iotests.py | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index e6f9f62b2b..010bca526c 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -359,12 +359,9 @@ def log(msg, filters=(), indent=None):
 for flt in filters:
 msg = flt(msg)
 if isinstance(msg, (dict, list)):
-# Python < 3.4 needs to know not to add whitespace when 
pretty-printing:
-separators = (', ', ': ') if indent is None else (',', ': ')
 # Don't sort if it's already sorted
 do_sort = not isinstance(msg, OrderedDict)
-print(json.dumps(msg, sort_keys=do_sort,
- indent=indent, separators=separators))
+print(json.dumps(msg, sort_keys=do_sort, indent=indent))
 else:
 print(msg)
 
-- 
2.21.1

[PATCH v10 05/14] iotests: add pylintrc file

[John Snow]

This allows others to get repeatable results with pylint. If you run
`pylint iotests.py`, you should see a 100% pass.

Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/pylintrc | 22 ++
 1 file changed, 22 insertions(+)
 create mode 100644 tests/qemu-iotests/pylintrc

diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc
new file mode 100644
index 00..daec2c4942
--- /dev/null
+++ b/tests/qemu-iotests/pylintrc
@@ -0,0 +1,22 @@
+[MESSAGES CONTROL]
+
+# Disable the message, report, category or checker with the given id(s). You
+# can either give multiple identifiers separated by comma (,) or put this
+# option multiple times (only on the command line, not in the configuration
+# file where it should appear only once). You can also use "--disable=all" to
+# disable everything first and then reenable specific checks. For example, if
+# you want to run only the similarities checker, you can use "--disable=all
+# --enable=similarities". If you want to run only the classes checker, but have
+# no Warning level messages displayed, use "--disable=all --enable=classes
+# --disable=W".
+disable=invalid-name,
+no-else-return,
+too-few-public-methods,
+too-many-arguments,
+too-many-branches,
+too-many-lines,
+too-many-locals,
+too-many-public-methods,
+# These are temporary, and should be removed:
+line-too-long,
+missing-docstring,
-- 
2.21.1

[PATCH v10 12/14] iotest 258: use script_main

[John Snow]

Since this one is nicely factored to use a single entry point,
use script_main to run the tests.

Signed-off-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/258 | 11 ---
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/tests/qemu-iotests/258 b/tests/qemu-iotests/258
index a65151dda6..e305a1502f 100755
--- a/tests/qemu-iotests/258
+++ b/tests/qemu-iotests/258
@@ -23,12 +23,6 @@ import iotests
 from iotests import log, qemu_img, qemu_io_silent, \
 filter_qmp_testfiles, filter_qmp_imgfmt
 
-# Need backing file and change-backing-file support
-iotests.script_initialize(
-supported_fmts=['qcow2', 'qed'],
-supported_platforms=['linux'],
-)
-
 # Returns a node for blockdev-add
 def node(node_name, path, backing=None, fmt=None, throttle=None):
 if fmt is None:
@@ -161,4 +155,7 @@ def main():
 test_concurrent_finish(False)
 
 if __name__ == '__main__':
-main()
+# Need backing file and change-backing-file support
+iotests.script_main(main,
+supported_fmts=['qcow2', 'qed'],
+supported_platforms=['linux'])
-- 
2.21.1

[PATCH v10 08/14] iotests: touch up log function signature

[John Snow]

Representing nested, recursive data structures in mypy is notoriously
difficult; the best we can reliably do right now is denote the leaf
types as "Any" while describing the general shape of the data.

Regardless, this fully annotates the log() function.

Typing notes:

TypeVar is a Type variable that can optionally be constrained by a
sequence of possible types. This variable is bound to a specific type
per-invocation, like a Generic.

log() behaves as log() now, where the incoming type informs the
signature it expects for any filter arguments passed in. If Msg is a
str, then filter should take and return a str.

Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/iotests.py | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 010bca526c..e2d3e89574 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -28,6 +28,7 @@
 import struct
 import subprocess
 import sys
+from typing import (Any, Callable, Dict, Iterable, List, Optional, TypeVar)
 import unittest
 
 # pylint: disable=import-error, wrong-import-position
@@ -353,9 +354,16 @@ def _filter(_key, value):
 return value
 return filter_qmp(qmsg, _filter)
 
-def log(msg, filters=(), indent=None):
-'''Logs either a string message or a JSON serializable message (like QMP).
-If indent is provided, JSON serializable messages are pretty-printed.'''
+
+Msg = TypeVar('Msg', Dict[str, Any], List[Any], str)
+
+def log(msg: Msg,
+filters: Iterable[Callable[[Msg], Msg]] = (),
+indent: Optional[int] = None) -> None:
+"""
+Logs either a string message or a JSON serializable message (like QMP).
+If indent is provided, JSON serializable messages are pretty-printed.
+"""
 for flt in filters:
 msg = flt(msg)
 if isinstance(msg, (dict, list)):
-- 
2.21.1

[PATCH v10 10/14] iotests: add hmp helper with logging

[John Snow]

Minor cleanup for HMP functions; helps with line length and consolidates
HMP helpers through one implementation function.

Although we are adding a universal toggle to turn QMP logging on or off,
many existing callers to hmp functions don't expect that output to be
logged, which causes quite a few changes in the test output.

For now, offer a use_log parameter.


Typing notes:

QMPResponse is just an alias for Dict[str, Any]. It holds no special
meanings and it is not a formal subtype of Dict[str, Any]. It is best
thought of as a lexical synonym.

We may well wish to add stricter subtypes in the future for certain
shapes of data that are not formalized as Python objects, at which point
we can simply retire the alias and allow mypy to more strictly check
usages of the name.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/iotests.py | 35 ++-
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index b08bcb87e1..dfc753c319 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -37,6 +37,10 @@
 
 assert sys.version_info >= (3, 6)
 
+# Type Aliases
+QMPResponse = Dict[str, Any]
+
+
 faulthandler.enable()
 
 # This will not work if arguments contain spaces but is necessary if we
@@ -540,25 +544,30 @@ def add_incoming(self, addr):
 self._args.append(addr)
 return self
 
-def pause_drive(self, drive, event=None):
-'''Pause drive r/w operations'''
+def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse:
+cmd = 'human-monitor-command'
+kwargs = {'command-line': command_line}
+if use_log:
+return self.qmp_log(cmd, **kwargs)
+else:
+return self.qmp(cmd, **kwargs)
+
+def pause_drive(self, drive: str, event: Optional[str] = None) -> None:
+"""Pause drive r/w operations"""
 if not event:
 self.pause_drive(drive, "read_aio")
 self.pause_drive(drive, "write_aio")
 return
-self.qmp('human-monitor-command',
- command_line='qemu-io %s "break %s bp_%s"'
- % (drive, event, drive))
+self.hmp(f'qemu-io {drive} "break {event} bp_{drive}"')
 
-def resume_drive(self, drive):
-self.qmp('human-monitor-command',
- command_line='qemu-io %s "remove_break bp_%s"'
- % (drive, drive))
+def resume_drive(self, drive: str) -> None:
+"""Resume drive r/w operations"""
+self.hmp(f'qemu-io {drive} "remove_break bp_{drive}"')
 
-def hmp_qemu_io(self, drive, cmd):
-'''Write to a given drive using an HMP command'''
-return self.qmp('human-monitor-command',
-command_line='qemu-io %s "%s"' % (drive, cmd))
+def hmp_qemu_io(self, drive: str, cmd: str,
+use_log: bool = False) -> QMPResponse:
+"""Write to a given drive using an HMP command"""
+return self.hmp(f'qemu-io {drive} "{cmd}"', use_log=use_log)
 
 def flatten_qmp_object(self, obj, output=None, basestr=''):
 if output is None:
-- 
2.21.1

[PATCH v10 09/14] iotests: limit line length to 79 chars

[John Snow]

79 is the PEP8 recommendation. This recommendation works well for
reading patch diffs in TUI email clients.

Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/iotests.py | 64 +++
 tests/qemu-iotests/pylintrc   |  6 +++-
 2 files changed, 47 insertions(+), 23 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index e2d3e89574..b08bcb87e1 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -80,9 +80,11 @@
 def qemu_img(*args):
 '''Run qemu-img and return the exit code'''
 devnull = open('/dev/null', 'r+')
-exitcode = subprocess.call(qemu_img_args + list(args), stdin=devnull, 
stdout=devnull)
+exitcode = subprocess.call(qemu_img_args + list(args),
+   stdin=devnull, stdout=devnull)
 if exitcode < 0:
-sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' 
'.join(qemu_img_args + list(args
+sys.stderr.write('qemu-img received signal %i: %s\n'
+ % (-exitcode, ' '.join(qemu_img_args + list(args
 return exitcode
 
 def ordered_qmp(qmsg, conv_keys=True):
@@ -121,7 +123,8 @@ def qemu_img_verbose(*args):
 '''Run qemu-img without suppressing its output and return the exit code'''
 exitcode = subprocess.call(qemu_img_args + list(args))
 if exitcode < 0:
-sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' 
'.join(qemu_img_args + list(args
+sys.stderr.write('qemu-img received signal %i: %s\n'
+ % (-exitcode, ' '.join(qemu_img_args + list(args
 return exitcode
 
 def qemu_img_pipe(*args):
@@ -132,7 +135,8 @@ def qemu_img_pipe(*args):
 universal_newlines=True)
 exitcode = subp.wait()
 if exitcode < 0:
-sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' 
'.join(qemu_img_args + list(args
+sys.stderr.write('qemu-img received signal %i: %s\n'
+ % (-exitcode, ' '.join(qemu_img_args + list(args
 return subp.communicate()[0]
 
 def qemu_img_log(*args):
@@ -162,7 +166,8 @@ def qemu_io(*args):
 universal_newlines=True)
 exitcode = subp.wait()
 if exitcode < 0:
-sys.stderr.write('qemu-io received signal %i: %s\n' % (-exitcode, ' 
'.join(args)))
+sys.stderr.write('qemu-io received signal %i: %s\n'
+ % (-exitcode, ' '.join(args)))
 return subp.communicate()[0]
 
 def qemu_io_log(*args):
@@ -284,10 +289,13 @@ def filter_test_dir(msg):
 def filter_win32(msg):
 return win32_re.sub("", msg)
 
-qemu_io_re = re.compile(r"[0-9]* ops; [0-9\/:. sec]* \([0-9\/.inf]* 
[EPTGMKiBbytes]*\/sec and [0-9\/.inf]* ops\/sec\)")
+qemu_io_re = re.compile(r"[0-9]* ops; [0-9\/:. sec]* "
+r"\([0-9\/.inf]* [EPTGMKiBbytes]*\/sec "
+r"and [0-9\/.inf]* ops\/sec\)")
 def filter_qemu_io(msg):
 msg = filter_win32(msg)
-return qemu_io_re.sub("X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)", 
msg)
+return qemu_io_re.sub("X ops; XX:XX:XX.X "
+  "(XXX YYY/sec and XXX ops/sec)", msg)
 
 chown_re = re.compile(r"chown [0-9]+:[0-9]+")
 def filter_chown(msg):
@@ -339,7 +347,9 @@ def filter_img_info(output, filename):
 line = line.replace(filename, 'TEST_IMG') \
.replace(imgfmt, 'IMGFMT')
 line = re.sub('iters: [0-9]+', 'iters: XXX', line)
-line = re.sub('uuid: [-a-f0-9]+', 'uuid: 
----', line)
+line = re.sub('uuid: [-a-f0-9]+',
+  'uuid: ----',
+  line)
 line = re.sub('cid: [0-9]+', 'cid: XX', line)
 lines.append(line)
 return '\n'.join(lines)
@@ -537,11 +547,13 @@ def pause_drive(self, drive, event=None):
 self.pause_drive(drive, "write_aio")
 return
 self.qmp('human-monitor-command',
- command_line='qemu-io %s "break %s bp_%s"' % (drive, event, 
drive))
+ command_line='qemu-io %s "break %s bp_%s"'
+ % (drive, event, drive))
 
 def resume_drive(self, drive):
 self.qmp('human-monitor-command',
- command_line='qemu-io %s "remove_break bp_%s"' % (drive, 
drive))
+ command_line='qemu-io %s "remove_break bp_%s"'
+ % (drive, drive))
 
 def hmp_qemu_io(self, drive, cmd):
 '''Write to a given drive using an HMP command'''
@@ -801,16 +813,18 @@ def dictpath(self, d, path):
 idx = int(idx)
 
 if not isinstance(d, dict) or component not in d:
-self.fail('failed path traversal for "%s" in "%s"' % (path, 
str(d)))
+self.fail(f'failed path traversal for "{path}" in "{d}"')
 d = d[component]
 
   

[PATCH v10 04/14] iotests: replace mutable list default args

[John Snow]

It's bad hygiene: if we modify this list, it will be modified across all
invocations.

(Remaining bad usages are fixed in a subsequent patch which changes the
function signature anyway.)

Signed-off-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/iotests.py | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 4d848339a5..51f84475d9 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -139,7 +139,7 @@ def qemu_img_log(*args):
 log(result, filters=[filter_testfiles])
 return result
 
-def img_info_log(filename, filter_path=None, imgopts=False, extra_args=[]):
+def img_info_log(filename, filter_path=None, imgopts=False, extra_args=()):
 args = ['info']
 if imgopts:
 args.append('--image-opts')
@@ -353,7 +353,7 @@ def _filter(_key, value):
 return value
 return filter_qmp(qmsg, _filter)
 
-def log(msg, filters=[], indent=None):
+def log(msg, filters=(), indent=None):
 '''Logs either a string message or a JSON serializable message (like QMP).
 If indent is provided, JSON serializable messages are pretty-printed.'''
 for flt in filters:
@@ -569,7 +569,7 @@ def get_qmp_events_filtered(self, wait=60.0):
 result.append(filter_qmp_event(ev))
 return result
 
-def qmp_log(self, cmd, filters=[], indent=None, **kwargs):
+def qmp_log(self, cmd, filters=(), indent=None, **kwargs):
 full_cmd = OrderedDict((
 ("execute", cmd),
 ("arguments", ordered_qmp(kwargs))
@@ -973,7 +973,7 @@ def case_notrun(reason):
 open('%s/%s.casenotrun' % (output_dir, seq), 'a').write(
 '[case not run] ' + reason + '\n')
 
-def verify_image_format(supported_fmts=[], unsupported_fmts=[]):
+def verify_image_format(supported_fmts=(), unsupported_fmts=()):
 assert not (supported_fmts and unsupported_fmts)
 
 if 'generic' in supported_fmts and \
@@ -987,7 +987,7 @@ def verify_image_format(supported_fmts=[], 
unsupported_fmts=[]):
 if not_sup or (imgfmt in unsupported_fmts):
 notrun('not suitable for this image format: %s' % imgfmt)
 
-def verify_protocol(supported=[], unsupported=[]):
+def verify_protocol(supported=(), unsupported=()):
 assert not (supported and unsupported)
 
 if 'generic' in supported:
@@ -1006,11 +1006,11 @@ def verify_platform(supported=None, unsupported=None):
 if not any((sys.platform.startswith(x) for x in supported)):
 notrun('not suitable for this OS: %s' % sys.platform)
 
-def verify_cache_mode(supported_cache_modes=[]):
+def verify_cache_mode(supported_cache_modes=()):
 if supported_cache_modes and (cachemode not in supported_cache_modes):
 notrun('not suitable for this cache mode: %s' % cachemode)
 
-def verify_aio_mode(supported_aio_modes=[]):
+def verify_aio_mode(supported_aio_modes=()):
 if supported_aio_modes and (aiomode not in supported_aio_modes):
 notrun('not suitable for this aio mode: %s' % aiomode)
 
@@ -1050,7 +1050,7 @@ def supported_formats(read_only=False):
 
 return supported_formats.formats[read_only]
 
-def skip_if_unsupported(required_formats=[], read_only=False):
+def skip_if_unsupported(required_formats=(), read_only=False):
 '''Skip Test Decorator
Runs the test if all the required formats are whitelisted'''
 def skip_test_decorator(func):
@@ -1101,11 +1101,11 @@ def execute_unittest(output, verbosity, debug):
 sys.stderr.write(out)
 
 def execute_test(test_function=None,
- supported_fmts=[],
+ supported_fmts=(),
  supported_platforms=None,
- supported_cache_modes=[], supported_aio_modes={},
- unsupported_fmts=[], supported_protocols=[],
- unsupported_protocols=[]):
+ supported_cache_modes=(), supported_aio_modes=(),
+ unsupported_fmts=(), supported_protocols=(),
+ unsupported_protocols=()):
 """Run either unittest or script-style tests."""
 
 # We are using TEST_DIR and QEMU_DEFAULT_MACHINE as proxies to
-- 
2.21.1

[PATCH v10 02/14] iotests: don't use 'format' for drive_add

[John Snow]

It shadows (with a different type) the built-in format.
Use something else.

Signed-off-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/055| 3 ++-
 tests/qemu-iotests/iotests.py | 6 +++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index 82b9f5f47d..4175fff5e4 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -469,7 +469,8 @@ class TestDriveCompression(iotests.QMPTestCase):
 qemu_img('create', '-f', fmt, blockdev_target_img,
  str(TestDriveCompression.image_len), *args)
 if attach_target:
-self.vm.add_drive(blockdev_target_img, format=fmt, 
interface="none")
+self.vm.add_drive(blockdev_target_img,
+  img_format=fmt, interface="none")
 
 self.vm.launch()
 
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 0641bbbc1f..a026d3343e 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -485,21 +485,21 @@ def add_drive_raw(self, opts):
 self._args.append(opts)
 return self
 
-def add_drive(self, path, opts='', interface='virtio', format=imgfmt):
+def add_drive(self, path, opts='', interface='virtio', img_format=imgfmt):
 '''Add a virtio-blk drive to the VM'''
 options = ['if=%s' % interface,
'id=drive%d' % self._num_drives]
 
 if path is not None:
 options.append('file=%s' % path)
-options.append('format=%s' % format)
+options.append('format=%s' % img_format)
 options.append('cache=%s' % cachemode)
 options.append('aio=%s' % aiomode)
 
 if opts:
 options.append(opts)
 
-if format == 'luks' and 'key-secret' not in opts:
+if img_format == 'luks' and 'key-secret' not in opts:
 # default luks support
 if luks_default_secret_object not in self._args:
 self.add_object(luks_default_secret_object)
-- 
2.21.1

[PATCH v10 01/14] iotests: do a light delinting

[John Snow]

This doesn't fix everything in here, but it does help clean up the
pylint report considerably.

This should be 100% style changes only; the intent is to make pylint
more useful by working on establishing a baseline for iotests that we
can gate against in the future.

Signed-off-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/iotests.py | 83 ++-
 1 file changed, 43 insertions(+), 40 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 7bc4934cd2..0641bbbc1f 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -16,11 +16,9 @@
 # along with this program.  If not, see .
 #
 
-import errno
 import os
 import re
 import subprocess
-import string
 import unittest
 import sys
 import struct
@@ -35,7 +33,7 @@
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
 from qemu import qtest
 
-assert sys.version_info >= (3,6)
+assert sys.version_info >= (3, 6)
 
 faulthandler.enable()
 
@@ -141,11 +139,11 @@ def qemu_img_log(*args):
 return result
 
 def img_info_log(filename, filter_path=None, imgopts=False, extra_args=[]):
-args = [ 'info' ]
+args = ['info']
 if imgopts:
 args.append('--image-opts')
 else:
-args += [ '-f', imgfmt ]
+args += ['-f', imgfmt]
 args += extra_args
 args.append(filename)
 
@@ -224,7 +222,7 @@ def cmd(self, cmd):
 # quit command is in close(), '\n' is added automatically
 assert '\n' not in cmd
 cmd = cmd.strip()
-assert cmd != 'q' and cmd != 'quit'
+assert cmd not in ('q', 'quit')
 self._p.stdin.write(cmd + '\n')
 self._p.stdin.flush()
 return self._read_output()
@@ -246,10 +244,8 @@ def qemu_nbd_early_pipe(*args):
 sys.stderr.write('qemu-nbd received signal %i: %s\n' %
  (-exitcode,
   ' '.join(qemu_nbd_args + ['--fork'] + list(args
-if exitcode == 0:
-return exitcode, ''
-else:
-return exitcode, subp.communicate()[0]
+
+return exitcode, subp.communicate()[0] if exitcode else ''
 
 def qemu_nbd_popen(*args):
 '''Run qemu-nbd in daemon mode and return the parent's exit code'''
@@ -313,7 +309,7 @@ def filter_qmp(qmsg, filter_fn):
 items = qmsg.items()
 
 for k, v in items:
-if isinstance(v, list) or isinstance(v, dict):
+if isinstance(v, (dict, list)):
 qmsg[k] = filter_qmp(v, filter_fn)
 else:
 qmsg[k] = filter_fn(k, v)
@@ -324,7 +320,7 @@ def filter_testfiles(msg):
 return msg.replace(prefix, 'TEST_DIR/PID-')
 
 def filter_qmp_testfiles(qmsg):
-def _filter(key, value):
+def _filter(_key, value):
 if is_str(value):
 return filter_testfiles(value)
 return value
@@ -350,7 +346,7 @@ def filter_imgfmt(msg):
 return msg.replace(imgfmt, 'IMGFMT')
 
 def filter_qmp_imgfmt(qmsg):
-def _filter(key, value):
+def _filter(_key, value):
 if is_str(value):
 return filter_imgfmt(value)
 return value
@@ -361,7 +357,7 @@ def log(msg, filters=[], indent=None):
 If indent is provided, JSON serializable messages are pretty-printed.'''
 for flt in filters:
 msg = flt(msg)
-if isinstance(msg, dict) or isinstance(msg, list):
+if isinstance(msg, (dict, list)):
 # Python < 3.4 needs to know not to add whitespace when 
pretty-printing:
 separators = (', ', ': ') if indent is None else (',', ': ')
 # Don't sort if it's already sorted
@@ -372,14 +368,14 @@ def log(msg, filters=[], indent=None):
 print(msg)
 
 class Timeout:
-def __init__(self, seconds, errmsg = "Timeout"):
+def __init__(self, seconds, errmsg="Timeout"):
 self.seconds = seconds
 self.errmsg = errmsg
 def __enter__(self):
 signal.signal(signal.SIGALRM, self.timeout)
 signal.setitimer(signal.ITIMER_REAL, self.seconds)
 return self
-def __exit__(self, type, value, traceback):
+def __exit__(self, exc_type, value, traceback):
 signal.setitimer(signal.ITIMER_REAL, 0)
 return False
 def timeout(self, signum, frame):
@@ -388,7 +384,7 @@ def timeout(self, signum, frame):
 def file_pattern(name):
 return "{0}-{1}".format(os.getpid(), name)
 
-class FilePaths(object):
+class FilePaths:
 """
 FilePaths is an auto-generated filename that cleans itself up.
 
@@ -535,11 +531,11 @@ def pause_drive(self, drive, event=None):
 self.pause_drive(drive, "write_aio")
 return
 self.qmp('human-monitor-command',
-command_line='qemu-io %s "break %s bp_%s"' % (drive, 
event, drive))
+ command_line='qemu-io %s "break %s bp_%s"' % (drive, event, 
drive))
 
 def resume_drive(self, drive):
 

[PATCH v10 00/14] iotests: use python logging

[John Snow]

This series uses python logging to enable output conditionally on
iotests.log(). We unify an initialization call (which also enables
debugging output for those tests with -d) and then make the switch
inside of iotests.

It will help alleviate the need to create logged/unlogged versions
of all the various helpers we have made.

Also, I got lost and accidentally delinted iotests while I was here.
Sorry about that. By version 9, it's now the overwhelming focus of
this series. No good deed, etc.

V10:

001/14:[0004] [FC] 'iotests: do a light delinting'
002/14:[] [--] 'iotests: don't use 'format' for drive_add'
003/14:[] [--] 'iotests: ignore import warnings from pylint'
004/14:[] [--] 'iotests: replace mutable list default args'
005/14:[0006] [FC] 'iotests: add pylintrc file'
006/14:[] [--] 'iotests: alphabetize standard imports'
007/14:[] [--] 'iotests: drop pre-Python 3.4 compatibility code'
008/14:[] [--] 'iotests: touch up log function signature'
009/14:[] [-C] 'iotests: limit line length to 79 chars'
010/14:[0009] [FC] 'iotests: add hmp helper with logging'
011/14:[0019] [FC] 'iotests: add script_initialize'
012/14:[] [--] 'iotest 258: use script_main'
013/14:[0013] [FC] 'iotests: Mark verify functions as private'
014/14:[0001] [FC] 'iotests: use python logging for iotests.log()'

001: replace "atom" name with "item". Kept RBs.
005: Alphabetized excluded warnings list. Kept RBs.
 Kevin's comments addressed by using pylint >= 2.2.0
009: Added Max's RB.
 Updated commit message based on Max's response
 Kevin's comments addressed by mypy >= 0.620
010: Fixed type hints (Kevin)
011: Replace 'Collection' with 'Sequence' to work around pylint/python 3.6
013: Update type signatures of _verify functions (Kevin)
014: Minor whitespace changes as the fault handler gets shuffled around.

V9:
006: New.
007: Split from old patch.
008: Split from old patch; enhanced a little to justify its own patch.
010: New, pulled in from bitmap-populate series. Helps line length.
011: Reflow columns for long `typing` import list. (Kept RB.)
014: New blank line. (Kept RB.)

V8:
- Split out the little drop of Python 3.4 code. (Phil)
- Change line continuation styles (QEMU Memorial Choir)
- Rebase changes; remove use_log from more places, adjust test output.

V7:
- All delinting patches are now entirely front-loaded.
- Redid delinting to avoid "correcting" no-else-return statements.
- Moved more mutable list corrections into patch 4, to make it standalone.
- Moved pylintrc up to patch 5. Disabled no-else-return.
- Added patch 6 to require line length checks.
  (Some python 3.4 compatibility code is removed as a consequence.)
- Patch 7 changes slightly as a result of patch 4 changes.
- Added some logging explainer into patch 10.
  (Patch changes slightly because of patch 6.)

V6:
 - It's been so long since V5, let's just look at it anew.

John Snow (14):
  iotests: do a light delinting
  iotests: don't use 'format' for drive_add
  iotests: ignore import warnings from pylint
  iotests: replace mutable list default args
  iotests: add pylintrc file
  iotests: alphabetize standard imports
  iotests: drop pre-Python 3.4 compatibility code
  iotests: touch up log function signature
  iotests: limit line length to 79 chars
  iotests: add hmp helper with logging
  iotests: add script_initialize
  iotest 258: use script_main
  iotests: Mark verify functions as private
  iotests: use python logging for iotests.log()

 tests/qemu-iotests/030|   4 +-
 tests/qemu-iotests/055|   3 +-
 tests/qemu-iotests/149|   3 +-
 tests/qemu-iotests/155|   2 +-
 tests/qemu-iotests/194|   4 +-
 tests/qemu-iotests/202|   4 +-
 tests/qemu-iotests/203|   4 +-
 tests/qemu-iotests/206|   2 +-
 tests/qemu-iotests/207|   6 +-
 tests/qemu-iotests/208|   2 +-
 tests/qemu-iotests/209|   2 +-
 tests/qemu-iotests/210|   6 +-
 tests/qemu-iotests/211|   6 +-
 tests/qemu-iotests/212|   6 +-
 tests/qemu-iotests/213|   6 +-
 tests/qemu-iotests/216|   4 +-
 tests/qemu-iotests/218|   2 +-
 tests/qemu-iotests/219|   2 +-
 tests/qemu-iotests/222|   7 +-
 tests/qemu-iotests/224|   4 +-
 tests/qemu-iotests/228|   6 +-
 tests/qemu-iotests/234|   4 +-
 tests/qemu-iotests/235|   4 +-
 tests/qemu-iotests/236|   2 +-
 tests/qemu-iotests/237|   2 +-
 tests/qemu-iotests/238|   2 +
 tests/qemu-iotests/242|   2 +-
 tests/qemu-iotests/245|   1 +
 tests/qemu-iotests/245.out|  24 +--
 tests/qemu-iotests/246|   2 +-
 tests/qemu-iotests/248|   2 +-
 tests/qemu-iotests/254|   2 +-
 tests/qemu-iotests/255|   2 +-
 tests/qemu-iotests/256|   2 +-
 tests/qemu-iotests/258|  10 +-
 tests/qemu-iotests/260|   4 +-
 tests/qemu-iotests/262|   4 +-
 tests/qemu-iotests/264|   4 

[PATCH v10 03/14] iotests: ignore import warnings from pylint

[John Snow]

The right way to solve this is to come up with a virtual environment
infrastructure that sets all the paths correctly, and/or to create
installable python modules that can be imported normally.

That's hard, so just silence this error for now.

Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Max Reitz 
Signed-off-by: John Snow 
---
 tests/qemu-iotests/iotests.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index a026d3343e..4d848339a5 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -30,6 +30,7 @@
 from collections import OrderedDict
 import faulthandler
 
+# pylint: disable=import-error, wrong-import-position
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
 from qemu import qtest
 
-- 
2.21.1

Re: [PATCH v9 00/14] iotests: use python logging

[John Snow]

On 3/30/20 9:00 AM, Max Reitz wrote:
> On 25.03.20 00:20, John Snow wrote:
>> This series uses python logging to enable output conditionally on
>> iotests.log(). We unify an initialization call (which also enables
>> debugging output for those tests with -d) and then make the switch
>> inside of iotests.
>>
>> It will help alleviate the need to create logged/unlogged versions
>> of all the various helpers we have made.
>>
>> Also, I got lost and accidentally delinted iotests while I was here.
>> Sorry about that. By version 9, it's now the overwhelming focus of
>> this series. No good deed, etc.
> 
> Generally, test patches are fair game for the freeze period.  However,
> this series is quite extensive, so I might prefer block-next here.
> OTOH, if I do take it to block-next, patch 11 might grow stale.
> 
> Do you have a strong opinion either way?
> 
> Max
> 

Not really, no.

Might be nice to see it tossed into the RC while everyone is doing
testing to see if it causes problems. On the other hand, it probably
WILL cause problems with various untested version combinations and so forth.

Either or.

--js

Re: [PATCH RESEND v4] nvme: introduce PMR support from NVMe 1.4 spec

[Klaus Birkelund Jensen]

On Mar 31 03:13, Keith Busch wrote:
> On Mon, Mar 30, 2020 at 08:07:32PM +0200, Klaus Birkelund Jensen wrote:
> > On Mar 31 01:55, Keith Busch wrote:
> > > On Mon, Mar 30, 2020 at 09:46:56AM -0700, Andrzej Jakowski wrote:
> > > > This patch introduces support for PMR that has been defined as part of 
> > > > NVMe 1.4
> > > > spec. User can now specify a pmrdev option that should point to 
> > > > HostMemoryBackend.
> > > > pmrdev memory region will subsequently be exposed as PCI BAR 2 in 
> > > > emulated NVMe
> > > > device. Guest OS can perform mmio read and writes to the PMR region 
> > > > that will stay
> > > > persistent across system reboot.
> > > 
> > > Looks pretty good to me.
> > > 
> > > Reviewed-by: Keith Busch 
> > > 
> > > For a possible future extention, it could be interesting to select the
> > > BAR for PMR dynamically, so that you could have CMB and PMR enabled on
> > > the same device.
> > > 
> > 
> > I thought about the same thing, but this would require disabling MSI-X
> > right? Otherwise there is not enough room. AFAIU, the iomem takes up
> > BAR0 and BAR1, CMB (or PMR) uses BAR2 and BAR3 and MSI-X uses BAR4 or 5.
> 
> That's the way the emulated device is currently set, but there's no
> reason for CMB or MSIx to use an exlusive bar. Both of those can be
> be offsets into BAR 0/1, for example. PMR is the only one that can't
> share.
> 

Oh, I did not consider that at all. Thanks!

Re: [PATCH v9 09/14] iotests: limit line length to 79 chars

[John Snow]

On 3/30/20 8:34 AM, Max Reitz wrote:
> On 30.03.20 14:31, Max Reitz wrote:
>> On 25.03.20 00:20, John Snow wrote:
>>> 79 is the PEP8 recommendation. This recommendation works well for
>>> reading patch diffs in TUI email clients.
>>>
>>> Signed-off-by: John Snow 
>>> ---
>>>  tests/qemu-iotests/iotests.py | 64 +++
>>>  tests/qemu-iotests/pylintrc   |  6 +++-
>>>  2 files changed, 47 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>> index 3a049ece5b..e12d6e533e 100644
>>> --- a/tests/qemu-iotests/iotests.py
>>> +++ b/tests/qemu-iotests/iotests.py
>>
>> [...]
>>
>>> @@ -537,11 +547,13 @@ def pause_drive(self, drive, event=None):
>>>  self.pause_drive(drive, "write_aio")
>>>  return
>>>  self.qmp('human-monitor-command',
>>> - command_line='qemu-io %s "break %s bp_%s"' % (drive, 
>>> event, drive))
>>> + command_line='qemu-io %s "break %s bp_%s"'
>>> + % (drive, event, drive))
>>>  
>>>  def resume_drive(self, drive):
>>>  self.qmp('human-monitor-command',
>>> - command_line='qemu-io %s "remove_break bp_%s"' % (drive, 
>>> drive))
>>> + command_line='qemu-io %s "remove_break bp_%s"'
>>> + % (drive, drive))
>>
>> Hm.  You didn’t reply on my (second, insisting) question on what stops
>> us from introducing a variable here (hmp_cmd = 'qemu-io %s ...' % ...;
>> self.qmp('human-monitor-command', command_line=hmp_cmd)).
>>
>> :c
> 
> Ah!  The next patch is there to address my whining.
> 
> c:
> 
> Reviewed-by: Max Reitz 
> 

Yep, sorry. I had the patch written already in another branch so I just
pulled it in. I didn't ignore you!

--js

Re: [PATCH v9 00/14] iotests: use python logging

[John Snow]

On 3/24/20 7:20 PM, John Snow wrote:
> This series uses python logging to enable output conditionally on
> iotests.log(). We unify an initialization call (which also enables
> debugging output for those tests with -d) and then make the switch
> inside of iotests.
> 
> It will help alleviate the need to create logged/unlogged versions
> of all the various helpers we have made.
> 
> Also, I got lost and accidentally delinted iotests while I was here.
> Sorry about that. By version 9, it's now the overwhelming focus of
> this series. No good deed, etc.


Version requirements, as discovered by Kevin's Python Museum:

mypy >= 0.620
pylint >= 2.2.0
astroid == 2.1.0 (or >= 2.2.0 if using pylint >= 2.3.0)


Hm, though ... pylint does not like 'Collection' very much:

iotests.py:1139:41: E1136: Value 'Collection' is unsubscriptable
(unsubscriptable-object)

It works OK for the same pylint versions under 3.7, but it's busted a
bit under 3.6. See https://github.com/PyCQA/pylint/issues/2377

Well. Collection is indeed the actual type we want (we need Iterable and
Container properties; i.e. supports 'for' and 'in'). There's no reason
to require a Sequence (adds Reversible and some notion of a fixed
ordering) -- but it will fix the typing problems in 3.6, so I'm going to
do that.



You can create a "minimum requirements" venv to test this:

> cd ~/src/qemu/tests/qemu-iotests/
> sudo dnf install python36
> pipenv --python 3.6
> pipenv shell
> pipenv install astroid==2.1.0 pylint==2.2.0 mypy==0.620
> pylint iotests.py
> set -x MYPYPATH ~/src/qemu/python/
> mypy --ignore-missing-imports iotests.py


(You can drop the --ignore-missing-imports if you are using mypy >= 0.650.)


--js

Re: [PATCH v9 08/14] iotests: touch up log function signature

[John Snow]

On 3/30/20 12:19 PM, Kevin Wolf wrote:
> Am 25.03.2020 um 00:20 hat John Snow geschrieben:
>> Representing nested, recursive data structures in mypy is notoriously
>> difficult; the best we can reliably do right now is denote the atom
>> types as "Any" while describing the general shape of the data.
>>
>> Regardless, this fully annotates the log() function.
>>
>> Typing notes:
>>
>> TypeVar is a Type variable that can optionally be constrained by a
>> sequence of possible types. This variable is bound per-invocation such
>> that the signature for filter=() requires that its callables take e.g. a
>> str and return a str.
>>
>> Signed-off-by: John Snow 
> 
> I like it. Does your version of mypy accept this? I actually get a
> warning that doesn't make sense to me:
> 
> iotests.py:392: error: Argument 1 to "info" of "Logger" has incompatible 
> type "Dict[str, Any]"; expected "str"
> 
> The code looks like this:
> 
> if isinstance(msg, (dict, list)):
> # Don't sort if it's already sorted
> do_sort = not isinstance(msg, OrderedDict)
> test_logger.info(json.dumps(msg, sort_keys=do_sort, indent=indent))
> else:
> test_logger.info(msg)
> 
> I have no idea why it would think it can still be Dict[str, Any] in the
> else branch. Even after adding an 'assert not instanceof(msg, dict), it
> still thinks so.
> 
> Probably time to update for me...
> 
> Kevin
> 

jsnow@probe ~/s/q/w/t/qemu-iotests (iotests-logging)> set -x MYPYPATH
~/src/qemu.git/work/python/
jsnow@probe ~/s/q/w/t/qemu-iotests (iotests-logging)> mypy iotests.py
jsnow@probe ~/s/q/w/t/qemu-iotests (iotests-logging)>


1. Mypy is stubborn and needs to be told exactly where it is allowed to
look for imports

2. works4me.

I use mypy 0.720, there are newer versions available, too.


oh, this is ... we need to look at the end of this series, not as of
this patch. Alright, let's look at patch 14:

0.600: Logger warning
0.610: Logger warning
0.620: OK
0.630: OK
0.650: OK
0.700: OK

Whatever this bug was seems to have been fixed since 0.620.

So let's say that I am targeting:

Python 3.6+
pylint 2.2.0+
mypy 0.620+

--js

Re: [PATCH RESEND v4] nvme: introduce PMR support from NVMe 1.4 spec

[Keith Busch]

On Mon, Mar 30, 2020 at 08:07:32PM +0200, Klaus Birkelund Jensen wrote:
> On Mar 31 01:55, Keith Busch wrote:
> > On Mon, Mar 30, 2020 at 09:46:56AM -0700, Andrzej Jakowski wrote:
> > > This patch introduces support for PMR that has been defined as part of 
> > > NVMe 1.4
> > > spec. User can now specify a pmrdev option that should point to 
> > > HostMemoryBackend.
> > > pmrdev memory region will subsequently be exposed as PCI BAR 2 in 
> > > emulated NVMe
> > > device. Guest OS can perform mmio read and writes to the PMR region that 
> > > will stay
> > > persistent across system reboot.
> > 
> > Looks pretty good to me.
> > 
> > Reviewed-by: Keith Busch 
> > 
> > For a possible future extention, it could be interesting to select the
> > BAR for PMR dynamically, so that you could have CMB and PMR enabled on
> > the same device.
> > 
> 
> I thought about the same thing, but this would require disabling MSI-X
> right? Otherwise there is not enough room. AFAIU, the iomem takes up
> BAR0 and BAR1, CMB (or PMR) uses BAR2 and BAR3 and MSI-X uses BAR4 or 5.

That's the way the emulated device is currently set, but there's no
reason for CMB or MSIx to use an exlusive bar. Both of those can be
be offsets into BAR 0/1, for example. PMR is the only one that can't
share.

Re: [PATCH v9 05/14] iotests: add pylintrc file

[John Snow]

On 3/30/20 11:49 AM, Kevin Wolf wrote:
> Am 25.03.2020 um 00:20 hat John Snow geschrieben:
>> This allows others to get repeatable results with pylint. If you run
>> `pylint iotests.py`, you should see a 100% pass.
>>
>> Signed-off-by: John Snow 
>> Reviewed-by: Max Reitz 
> 
> I see you said "should" and not "will", but I still get two warnings:
> 
> iotests.py:405:22: W0613: Unused argument 'signum' (unused-argument)
> iotests.py:405:30: W0613: Unused argument 'frame' (unused-argument)
> 
> Kevin
> 

I wasn't entirely sure if it'd work across all versions of pylint, all
versions of python, or what the tree would look like by the time the
patches got applied ... and I didn't see these warnings.


I rebased to origin/master today and with these first five patches applied:

jsnow@probe ~/s/q/w/t/qemu-iotests (iotests-logging)> pylint iotests.py


Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00)

jsnow@probe ~/s/q/w/t/qemu-iotests (iotests-logging)> pylint --version
pylint 2.3.1
astroid 2.2.5
Python 3.7.6 (default, Jan 30 2020, 10:29:04)
[GCC 9.2.1 20190827 (Red Hat 9.2.1-1)]


I upgraded to pylint 2.4.4 and that seems to work OK too.


So let's try this:

> sudo dnf install python36
> pipenv --python 3.6
> pipenv shell

> pipenv install pylint
> which pylint

(qemu-iotests) jsnow@probe ~/s/q/w/t/qemu-iotests (iotests-logging)>
which pylint
/home/jsnow/.local/share/virtualenvs/qemu-iotests-SREwaVl8/bin/pylint

(qemu-iotests) jsnow@probe ~/s/q/w/t/qemu-iotests (iotests-logging)>
pylint --version
pylint 2.4.4
astroid 2.3.3
Python 3.6.10 (default, Dec 27 2019, 13:40:13)
[GCC 9.2.1 20190827 (Red Hat 9.2.1-1)]


Python 3.6:
- Pylint 2.0.0 WARN
- Pylint 2.1.0 WARN
- Pylint 2.2.0 OK
- Pylint 2.4.4 OK
- Pylint 2.3.0 OK


Hm. Python 3.6 is from December 23, 2016 -- but Pylint from that era is
probably Pylint 1.6.5 from Jan 22, 2017.

That's probably ... obnoxiously old to support. Do we have any
viewpoints on minimum version requirements for CQA tools? Modern pylint
works just fine for Python 3.6, but I have no earthly idea if there are
reasons that "old" distributions with Python 3.6 can't get "modern"
pylint packages.

Gut feeling: Any Python 3.6 distribution has pip, and pip can get the
latest packages. It would be OK to require pylint >= 2.2.0, probably.


--js

Re: [PATCH RESEND v4] nvme: introduce PMR support from NVMe 1.4 spec

[Klaus Birkelund Jensen]

On Mar 31 01:55, Keith Busch wrote:
> On Mon, Mar 30, 2020 at 09:46:56AM -0700, Andrzej Jakowski wrote:
> > This patch introduces support for PMR that has been defined as part of NVMe 
> > 1.4
> > spec. User can now specify a pmrdev option that should point to 
> > HostMemoryBackend.
> > pmrdev memory region will subsequently be exposed as PCI BAR 2 in emulated 
> > NVMe
> > device. Guest OS can perform mmio read and writes to the PMR region that 
> > will stay
> > persistent across system reboot.
> 
> Looks pretty good to me.
> 
> Reviewed-by: Keith Busch 
> 
> For a possible future extention, it could be interesting to select the
> BAR for PMR dynamically, so that you could have CMB and PMR enabled on
> the same device.
> 

I thought about the same thing, but this would require disabling MSI-X
right? Otherwise there is not enough room. AFAIU, the iomem takes up
BAR0 and BAR1, CMB (or PMR) uses BAR2 and BAR3 and MSI-X uses BAR4 or 5.

Re: [RFC 0/3] 64bit block-layer part I

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20200330141818.31294-1-vsement...@virtuozzo.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing 
commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC  block/nbd.o
  CC  block/sheepdog.o
  CC  block/accounting.o
/tmp/qemu-test/src/block/file-win32.c:643:27: error: initialization of 
'BlockAIOCB * (*)(BlockDriverState *, int64_t,  int64_t,  QEMUIOVector *, int,  
void (*)(void *, int), void *)' {aka 'struct BlockAIOCB * (*)(struct 
BlockDriverState *, long long int,  long long int,  struct QEMUIOVector *, int, 
 void (*)(void *, int), void *)'} from incompatible pointer type 'BlockAIOCB * 
(*)(BlockDriverState *, uint64_t,  uint64_t,  QEMUIOVector *, int,  void 
(*)(void *, int), void *)' {aka 'struct BlockAIOCB * (*)(struct 
BlockDriverState *, long long unsigned int,  long long unsigned int,  struct 
QEMUIOVector *, int,  void (*)(void *, int), void *)'} 
[-Werror=incompatible-pointer-types]
 .bdrv_aio_preadv= raw_aio_preadv,
   ^~
/tmp/qemu-test/src/block/file-win32.c:643:27: note: (near initialization for 
'bdrv_file.bdrv_aio_preadv')
/tmp/qemu-test/src/block/file-win32.c:644:27: error: initialization of 
'BlockAIOCB * (*)(BlockDriverState *, int64_t,  int64_t,  QEMUIOVector *, int,  
void (*)(void *, int), void *)' {aka 'struct BlockAIOCB * (*)(struct 
BlockDriverState *, long long int,  long long int,  struct QEMUIOVector *, int, 
 void (*)(void *, int), void *)'} from incompatible pointer type 'BlockAIOCB * 
(*)(BlockDriverState *, uint64_t,  uint64_t,  QEMUIOVector *, int,  void 
(*)(void *, int), void *)' {aka 'struct BlockAIOCB * (*)(struct 
BlockDriverState *, long long unsigned int,  long long unsigned int,  struct 
QEMUIOVector *, int,  void (*)(void *, int), void *)'} 
[-Werror=incompatible-pointer-types]
 .bdrv_aio_pwritev   = raw_aio_pwritev,
   ^~~
/tmp/qemu-test/src/block/file-win32.c:644:27: note: (near initialization for 
'bdrv_file.bdrv_aio_pwritev')
/tmp/qemu-test/src/block/file-win32.c:812:27: error: initialization of 
'BlockAIOCB * (*)(BlockDriverState *, int64_t,  int64_t,  QEMUIOVector *, int,  
void (*)(void *, int), void *)' {aka 'struct BlockAIOCB * (*)(struct 
BlockDriverState *, long long int,  long long int,  struct QEMUIOVector *, int, 
 void (*)(void *, int), void *)'} from incompatible pointer type 'BlockAIOCB * 
(*)(BlockDriverState *, uint64_t,  uint64_t,  QEMUIOVector *, int,  void 
(*)(void *, int), void *)' {aka 'struct BlockAIOCB * (*)(struct 
BlockDriverState *, long long unsigned int,  long long unsigned int,  struct 
QEMUIOVector *, int,  void (*)(void *, int), void *)'} 
[-Werror=incompatible-pointer-types]
 .bdrv_aio_preadv= raw_aio_preadv,
   ^~
/tmp/qemu-test/src/block/file-win32.c:812:27: note: (near initialization for 
'bdrv_host_device.bdrv_aio_preadv')
/tmp/qemu-test/src/block/file-win32.c:813:27: error: initialization of 
'BlockAIOCB * (*)(BlockDriverState *, int64_t,  int64_t,  QEMUIOVector *, int,  
void (*)(void *, int), void *)' {aka 'struct BlockAIOCB * (*)(struct 
BlockDriverState *, long long int,  long long int,  struct QEMUIOVector *, int, 
 void (*)(void *, int), void *)'} from incompatible pointer type 'BlockAIOCB * 
(*)(BlockDriverState *, uint64_t,  uint64_t,  QEMUIOVector *, int,  void 
(*)(void *, int), void *)' {aka 'struct BlockAIOCB * (*)(struct 
BlockDriverState *, long long unsigned int,  long long unsigned int,  struct 
QEMUIOVector *, int,  void (*)(void *, int), void *)'} 
[-Werror=incompatible-pointer-types]
 .bdrv_aio_pwritev   = raw_aio_pwritev,
   ^~~
/tmp/qemu-test/src/block/file-win32.c:813:27: note: (near initialization for 
'bdrv_host_device.bdrv_aio_pwritev')
cc1: all warnings being treated as errors
make: *** [/tmp/qemu-test/src/rules.mak:69: block/file-win32.o] Error 1
make: *** Waiting for unfinished jobs
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in 
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
'--label', 'com.qemu.instance.uuid=46f0957c6a3a47e5971905cb30141048', '-u', 
'1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', 
'-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 
'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', 
'/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', 
'/var/tmp/patchew-tester-tmp-0szyhgs2/src/docker-src.2020-03-30-13.48.43.9903:/var/tmp/qemu:z,ro',
 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit 
status 2.

Re: [RFC 0/3] 64bit block-layer part I

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20200330141818.31294-1-vsement...@virtuozzo.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing 
commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC  tests/test-int128.o
  CC  tests/rcutorture.o
  CC  tests/test-rcu-list.o
/tmp/qemu-test/src/tests/test-block-iothread.c:68:5: error: initialization from 
incompatible pointer type [-Werror]
 .bdrv_co_preadv = bdrv_test_co_prwv,
 ^
/tmp/qemu-test/src/tests/test-block-iothread.c:68:5: error: (near 
initialization for 'bdrv_test.bdrv_co_preadv') [-Werror]
/tmp/qemu-test/src/tests/test-block-iothread.c:69:5: error: initialization from 
incompatible pointer type [-Werror]
 .bdrv_co_pwritev= bdrv_test_co_prwv,
 ^
/tmp/qemu-test/src/tests/test-block-iothread.c:69:5: error: (near 
initialization for 'bdrv_test.bdrv_co_pwritev') [-Werror]
cc1: all warnings being treated as errors
make: *** [tests/test-block-iothread.o] Error 1
make: *** Waiting for unfinished jobs
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in 
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
'--label', 'com.qemu.instance.uuid=ac3e5859c1b54c68988c07cda02958cd', '-u', 
'1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', 
'-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 
'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', 
'/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', 
'/var/tmp/patchew-tester-tmp-4cxd8d3k/src/docker-src.2020-03-30-13.44.40.29863:/var/tmp/qemu:z,ro',
 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit 
status 2.
filter=--filter=label=com.qemu.instance.uuid=ac3e5859c1b54c68988c07cda02958cd
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-4cxd8d3k/src'
make: *** [docker-run-test-quick@centos7] Error 2

real3m30.965s
user0m8.371s


The full log is available at
http://patchew.org/logs/20200330141818.31294-1-vsement...@virtuozzo.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

Re: [PATCH v9 08/14] iotests: touch up log function signature

[John Snow]

On 3/30/20 8:28 AM, Max Reitz wrote:
> On 25.03.20 00:20, John Snow wrote:
>> Representing nested, recursive data structures in mypy is notoriously
>> difficult; the best we can reliably do right now is denote the atom
>> types as "Any" while describing the general shape of the data.
>>
>> Regardless, this fully annotates the log() function.
>>
>> Typing notes:
>>
>> TypeVar is a Type variable that can optionally be constrained by a
>> sequence of possible types. This variable is bound per-invocation such
>> that the signature for filter=() requires that its callables take e.g. a
>> str and return a str.
> 
> So it works like a generic, except that its declaration isn’t part of
> and thus local to the function.  Interesting.  Not sure if I like it,
> but, well, it’s Python.  (Seems to me on the same level of “Interesting”
> as the “Don’t use an empty list as a default argument” thing.[1])
> 

Ah, right, yes -- it's a Generic. That's the word.

You can create ... generic annotations, like "T" that aren't limited to
any types at all, and use it wherever you want in subsequent
annotations. They're local to each annotation.

In this case, I limit the types of what it can be and give it a name.

>> Signed-off-by: John Snow 
>> ---
>>  tests/qemu-iotests/iotests.py | 14 +++---
>>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> Haha.  I can’t help but find it funny how my “I won’t complain, but I
> feel like some people might want this to be two patches” led to this “If
> I’m going to make a dedicated patch for this, I might as well go all the
> way” patch.  It’s quite cool.
> 

Sometimes you just know the next question is going to be "But why didn't
you annotate _THIS_ part?" and the answer "Because it's sort of fiddly
and kind of hard and I'm lazy" isn't going to fly too far...

> Reviewed-by: Max Reitz 
> 
> [1] I just read an article on that thing again (empty lists as default
> argument), which claims that this is in fact a very useful feature of
> Python.  This makes me want to go on another rant, but I’ve learned
> enough restraint by now not to.
> 

It's absurd and should be removed from the language. What this people
actually want is the concept of a static local.

But that's a C thing, so they'll pretend they definitely don't want it
for years until they come up with a new name for it, and then it will be
OK and it will be everyone's favorite new feature.

--js

Re: [RFC 0/3] 64bit block-layer part I

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20200330141818.31294-1-vsement...@virtuozzo.com/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

clang -iquote /tmp/qemu-test/build/tests -iquote tests -iquote 
/tmp/qemu-test/src/tcg/i386 -isystem /tmp/qemu-test/src/linux-headers -isystem 
/tmp/qemu-test/build/linux-headers -iquote . -iquote /tmp/qemu-test/src -iquote 
/tmp/qemu-test/src/accel/tcg -iquote /tmp/qemu-test/src/include -iquote 
/tmp/qemu-test/src/disas/libvixl -I/usr/include/pixman-1   -Werror 
-fsanitize=undefined -fsanitize=address  -pthread -I/usr/include/glib-2.0 
-I/usr/lib64/glib-2.0/include  -fPIE -DPIE -m64 -mcx16 -D_GNU_SOURCE 
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes 
-Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes 
-fno-strict-aliasing -fno-common -fwrapv -std=gnu99  -Wno-string-plus-int 
-Wno-typedef-redefinition -Wno-initializer-overrides -Wexpansion-to-defined 
-Wendif-labels -Wno-shift-negative-value -Wno-missing-include-dirs -Wempty-body 
-Wnested-externs -Wformat-security -Wformat-y2k -Winit-self 
-Wignored-qualifiers -Wold-style-definition -Wtype-limits 
-fstack-protector-strong   -I/usr/include/p11-kit-1   -DLEGACY_RDMA_REG_MR 
-DSTRUCT_IOVEC_DEFINED  -I/usr/include/libpng16  -I/usr/include/spice-1 
-I/usr/include/spice-server -I/usr/include/cacard -I/usr/include/glib-2.0 
-I/usr/lib64/glib-2.0/include -I/usr/include/nss3 -I/usr/include/nspr4 -pthread 
-I/usr/include/libmount -I/usr/include/blkid -I/usr/include/uuid 
-I/usr/include/pixman-1   -I/tmp/qemu-test/src/tests 
-I/tmp/qemu-test/src/tests/qtest -MMD -MP -MT tests/test-crypto-hash.o -MF 
tests/test-crypto-hash.d -g   -c -o tests/test-crypto-hash.o 
/tmp/qemu-test/src/tests/test-crypto-hash.c
clang -iquote /tmp/qemu-test/build/tests -iquote tests -iquote 
/tmp/qemu-test/src/tcg/i386 -isystem /tmp/qemu-test/src/linux-headers -isystem 
/tmp/qemu-test/build/linux-headers -iquote . -iquote /tmp/qemu-test/src -iquote 
/tmp/qemu-test/src/accel/tcg -iquote /tmp/qemu-test/src/include -iquote 
/tmp/qemu-test/src/disas/libvixl -I/usr/include/pixman-1   -Werror 
-fsanitize=undefined -fsanitize=address  -pthread -I/usr/include/glib-2.0 
-I/usr/lib64/glib-2.0/include  -fPIE -DPIE -m64 -mcx16 -D_GNU_SOURCE 
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes 
-Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes 
-fno-strict-aliasing -fno-common -fwrapv -std=gnu99  -Wno-string-plus-int 
-Wno-typedef-redefinition -Wno-initializer-overrides -Wexpansion-to-defined 
-Wendif-labels -Wno-shift-negative-value -Wno-missing-include-dirs -Wempty-body 
-Wnested-externs -Wformat-security -Wformat-y2k -Winit-self 
-Wignored-qualifiers -Wold-style-definition -Wtype-limits 
-fstack-protector-strong   -I/usr/include/p11-kit-1   -DLEGACY_RDMA_REG_MR 
-DSTRUCT_IOVEC_DEFINED  -I/usr/include/libpng16  -I/usr/include/spice-1 
-I/usr/include/spice-server -I/usr/include/cacard -I/usr/include/glib-2.0 
-I/usr/lib64/glib-2.0/include -I/usr/include/nss3 -I/usr/include/nspr4 -pthread 
-I/usr/include/libmount -I/usr/include/blkid -I/usr/include/uuid 
-I/usr/include/pixman-1   -I/tmp/qemu-test/src/tests 
-I/tmp/qemu-test/src/tests/qtest -MMD -MP -MT tests/test-crypto-hmac.o -MF 
tests/test-crypto-hmac.d -g   -c -o tests/test-crypto-hmac.o 
/tmp/qemu-test/src/tests/test-crypto-hmac.c
clang -iquote /tmp/qemu-test/build/tests -iquote tests -iquote 
/tmp/qemu-test/src/tcg/i386 -isystem /tmp/qemu-test/src/linux-headers -isystem 
/tmp/qemu-test/build/linux-headers -iquote . -iquote /tmp/qemu-test/src -iquote 
/tmp/qemu-test/src/accel/tcg -iquote /tmp/qemu-test/src/include -iquote 
/tmp/qemu-test/src/disas/libvixl -I/usr/include/pixman-1   -Werror 
-fsanitize=undefined -fsanitize=address  -pthread -I/usr/include/glib-2.0 
-I/usr/lib64/glib-2.0/include  -fPIE -DPIE -m64 -mcx16 -D_GNU_SOURCE 
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes 
-Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes 
-fno-strict-aliasing -fno-common -fwrapv -std=gnu99  -Wno-string-plus-int 
-Wno-typedef-redefinition -Wno-initializer-overrides -Wexpansion-to-defined 
-Wendif-labels -Wno-shift-negative-value -Wno-missing-include-dirs -Wempty-body 
-Wnested-externs -Wformat-security -Wformat-y2k -Winit-self 
-Wignored-qualifiers -Wold-style-definition -Wtype-limits 
-fstack-protector-strong   -I/usr/include/p11-kit-1   -DLEGACY_RDMA_REG_MR 
-DSTRUCT_IOVEC_DEFINED  -I/usr/include/libpng16  -I/usr/include/spice-1 
-I/usr/include/spice-server -I/usr/include/cacard -I/usr/include/glib-2.0 
-I/usr/lib64/glib-2.0/include -I/usr/include/nss3 -I/usr/include/nspr4 -pthread 
-I/usr/include/libmount 

Re: [PATCH v9 05/14] iotests: add pylintrc file

[John Snow]

On 3/30/20 10:45 AM, Markus Armbruster wrote:
> John Snow  writes:
> 
>> This allows others to get repeatable results with pylint. If you run
>> `pylint iotests.py`, you should see a 100% pass.
> 
> Nice.
> 
>>
>> Signed-off-by: John Snow 
>> Reviewed-by: Max Reitz 
>> ---
>>  tests/qemu-iotests/pylintrc | 22 ++
>>  1 file changed, 22 insertions(+)
>>  create mode 100644 tests/qemu-iotests/pylintrc
>>
>> diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc
>> new file mode 100644
>> index 00..8720b6a0de
>> --- /dev/null
>> +++ b/tests/qemu-iotests/pylintrc
>> @@ -0,0 +1,22 @@
>> +[MESSAGES CONTROL]
>> +
>> +# Disable the message, report, category or checker with the given id(s). You
>> +# can either give multiple identifiers separated by comma (,) or put this
>> +# option multiple times (only on the command line, not in the configuration
>> +# file where it should appear only once). You can also use "--disable=all" 
>> to
>> +# disable everything first and then reenable specific checks. For example, 
>> if
>> +# you want to run only the similarities checker, you can use "--disable=all
>> +# --enable=similarities". If you want to run only the classes checker, but 
>> have
>> +# no Warning level messages displayed, use "--disable=all --enable=classes
>> +# --disable=W".
>> +disable=invalid-name,
>> +no-else-return,
>> +too-many-lines,
>> +too-few-public-methods,
>> +too-many-arguments,
>> +too-many-locals,
>> +too-many-branches,
>> +too-many-public-methods,
> 
> Keep sorted?
> 

OK, (glances at email) if there's a good reason to respin. So far Kevin
asked me to change the word "atom" to "item" which I would consider a
"Maintainer, take mercy on me and just fix this up, would you?" fix, but
let's see how the rest of my email session goes today.

>> +# These are temporary, and should be removed:
>> +missing-docstring,
>> +line-too-long,
> 
> For what it's worth, I also disable these for checking QAPI code, except
> for no-else-return.  My true reason for keeping no-else-return is of
> course that I agree with pylint these elses are ugly.  But I pretend to
> simply go with the flow ;)
> 

You probably also have a few more of the "too-many" warnings disabled,
too. I was working on refactoring the parser recently and found quite a
few more there that were just not worth fixing.

As for the no-else-return ... I think this is direly legitimate:

if cond_a:
statement_1
statement_2
return statement_3
elif cond_b:
statement_4
statement_5
return statement_6
else:
statement_7
statement_8
return statement_9

When you've got at least three choices and neither one is particularly
"canonical", it can be nice to see them chunked out where they are
visually weighted equally.

That said, for this stuff:

if error_cond:
return -1
else:
return 0

Should probably be avoided, although ... it's fine.

I think no-else-return is just going to be an intense matter of style
and taste and cannot be left to the linter, so I think I agree with
leaving it on the disabled list.

--js

Re: [PATCH v9 01/14] iotests: do a light delinting

[John Snow]

On 3/30/20 10:39 AM, Markus Armbruster wrote:
> John Snow  writes:
> 
>> This doesn't fix everything in here, but it does help clean up the
>> pylint report considerably.
>>
>> This should be 100% style changes only; the intent is to make pylint
>> more useful by working on establishing a baseline for iotests that we
>> can gate against in the future.
>>
>> Signed-off-by: John Snow 
>> Reviewed-by: Philippe Mathieu-Daudé 
>> Reviewed-by: Max Reitz 
>> ---
>>  tests/qemu-iotests/iotests.py | 83 ++-
>>  1 file changed, 43 insertions(+), 40 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 7bc4934cd2..886ae962ae 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
> [...]
>> @@ -1062,6 +1063,7 @@ def func_wrapper(test_case: QMPTestCase, *args, 
>> **kwargs):
>>  if usf_list:
>>  test_case.case_skip('{}: formats {} are not 
>> whitelisted'.format(
>>  test_case, usf_list))
>> +return None
>>  else:
>>  return func(test_case, *args, **kwargs)
>>  return func_wrapper
>> @@ -1073,6 +1075,7 @@ def skip_if_user_is_root(func):
>>  def func_wrapper(*args, **kwargs):
>>  if os.getuid() == 0:
>>  case_notrun('{}: cannot be run as root'.format(args[0]))
>> +return None
>>  else:
>>  return func(*args, **kwargs)
>>  return func_wrapper
> 
> Observation, not demand: this trades one kind of pylint report for
> another: inconsistent-return-statements for no-else-return.  PATCH 05
> suppresses no-else-return.
> 

Hm, yeah. I think there was some light consensus that "no-else-return"
was perfectly fine, so this patch builds towards that specific pylintrc.

It isn't, in my opinion, a regression or lateral movement as there isn't
a pylintrc baseline yet.

--js

Re: [PATCH v9 01/14] iotests: do a light delinting

[John Snow]

On 3/30/20 11:41 AM, Kevin Wolf wrote:
> Am 25.03.2020 um 00:20 hat John Snow geschrieben:
>> This doesn't fix everything in here, but it does help clean up the
>> pylint report considerably.
>>
>> This should be 100% style changes only; the intent is to make pylint
>> more useful by working on establishing a baseline for iotests that we
>> can gate against in the future.
>>
>> Signed-off-by: John Snow 
>> Reviewed-by: Philippe Mathieu-Daudé 
>> Reviewed-by: Max Reitz 
> 
>> @@ -550,8 +546,8 @@ def flatten_qmp_object(self, obj, output=None, 
>> basestr=''):
>>  if output is None:
>>  output = dict()
>>  if isinstance(obj, list):
>> -for i in range(len(obj)):
>> -self.flatten_qmp_object(obj[i], output, basestr + str(i) + 
>> '.')
>> +for i, atom in enumerate(obj):
>> +self.flatten_qmp_object(atom, output, basestr + str(i) + 
>> '.')
> 
> I think atom isn't strictly the right word because we expect nested data
> structures (as shown by the recursive call). If I understand correctly,
> what Python calls things in lists is "items".
> 

I can't imagine how the philosophers felt when they discovered subatomic
particles.

--js

[PATCH RESEND v4] nvme: introduce PMR support from NVMe 1.4 spec

[Andrzej Jakowski]

This patch introduces support for PMR that has been defined as part of NVMe 1.4
spec. User can now specify a pmrdev option that should point to 
HostMemoryBackend.
pmrdev memory region will subsequently be exposed as PCI BAR 2 in emulated NVMe
device. Guest OS can perform mmio read and writes to the PMR region that will 
stay
persistent across system reboot.

Signed-off-by: Andrzej Jakowski 
Reviewed-by: Klaus Jensen 
Reviewed-by: Stefan Hajnoczi 
---
Changelog:

v4:
 - replaced qemu_msync() use with qemu_ram_writeback() to allow pmem_persist()
   or qemu_msync() be called depending on configuration [4] (Stefan)

 - rephrased comments to improve clarity and fixed code style issues [4]
   (Stefan, Klaus)

v3:
 - reworked PMR to use HostMemoryBackend instead of directly mapping PMR
   backend file into qemu [1] (Stefan)

v2:
 - provided support for Bit 1 from PMRWBM register instead of Bit 0 to ensure
   improved performance in virtualized environment [2] (Stefan)

 - added check if pmr size is power of two in size [3] (David)

 - addressed cross compilation build problems reported by CI environment

v1:
 - inital push of PMR emulation

[1]: 
https://lore.kernel.org/qemu-devel/20200306223853.37958-1-andrzej.jakow...@linux.intel.com/
[2]: 
https://nvmexpress.org/wp-content/uploads/NVM-Express-1_4-2019.06.10-Ratified.pdf
[3]: 
https://lore.kernel.org/qemu-devel/20200218224811.30050-1-andrzej.jakow...@linux.intel.com/
[4]: 
https://lore.kernel.org/qemu-devel/20200318200303.11322-1-andrzej.jakow...@linux.intel.com/
 
---
Persistent Memory Region (PMR) is a new optional feature provided in NVMe 1.4
specification. This patch implements initial support for it in NVMe driver.
---
 hw/block/Makefile.objs |   2 +-
 hw/block/nvme.c| 109 ++
 hw/block/nvme.h|   2 +
 hw/block/trace-events  |   4 +
 include/block/nvme.h   | 172 +
 5 files changed, 288 insertions(+), 1 deletion(-)

diff --git a/hw/block/Makefile.objs b/hw/block/Makefile.objs
index 4b4a2b338d..47960b5f0d 100644
--- a/hw/block/Makefile.objs
+++ b/hw/block/Makefile.objs
@@ -7,12 +7,12 @@ common-obj-$(CONFIG_PFLASH_CFI02) += pflash_cfi02.o
 common-obj-$(CONFIG_XEN) += xen-block.o
 common-obj-$(CONFIG_ECC) += ecc.o
 common-obj-$(CONFIG_ONENAND) += onenand.o
-common-obj-$(CONFIG_NVME_PCI) += nvme.o
 common-obj-$(CONFIG_SWIM) += swim.o
 
 common-obj-$(CONFIG_SH4) += tc58128.o
 
 obj-$(CONFIG_VIRTIO_BLK) += virtio-blk.o
 obj-$(CONFIG_VHOST_USER_BLK) += vhost-user-blk.o
+obj-$(CONFIG_NVME_PCI) += nvme.o
 
 obj-y += dataplane/
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index d28335cbf3..9b453423cf 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -19,10 +19,19 @@
  *  -drive file=,if=none,id=
  *  -device nvme,drive=,serial=,id=, \
  *  cmb_size_mb=, \
+ *  [pmrdev=,] \
  *  num_queues=
  *
  * Note cmb_size_mb denotes size of CMB in MB. CMB is assumed to be at
  * offset 0 in BAR2 and supports only WDS, RDS and SQS for now.
+ *
+ * cmb_size_mb= and pmrdev= options are mutually exclusive due to limitation
+ * in available BAR's. cmb_size_mb= will take precedence over pmrdev= when
+ * both provided.
+ * Enabling pmr emulation can be achieved by pointing to memory-backend-file.
+ * For example:
+ * -object memory-backend-file,id=,share=on,mem-path=, \
+ *  size=  -device nvme,...,pmrdev=
  */
 
 #include "qemu/osdep.h"
@@ -35,7 +44,9 @@
 #include "sysemu/sysemu.h"
 #include "qapi/error.h"
 #include "qapi/visitor.h"
+#include "sysemu/hostmem.h"
 #include "sysemu/block-backend.h"
+#include "exec/ram_addr.h"
 
 #include "qemu/log.h"
 #include "qemu/module.h"
@@ -1141,6 +1152,26 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, 
uint64_t data,
 NVME_GUEST_ERR(nvme_ub_mmiowr_cmbsz_readonly,
"invalid write to read only CMBSZ, ignored");
 return;
+case 0xE00: /* PMRCAP */
+NVME_GUEST_ERR(nvme_ub_mmiowr_pmrcap_readonly,
+   "invalid write to PMRCAP register, ignored");
+return;
+case 0xE04: /* TODO PMRCTL */
+break;
+case 0xE08: /* PMRSTS */
+NVME_GUEST_ERR(nvme_ub_mmiowr_pmrsts_readonly,
+   "invalid write to PMRSTS register, ignored");
+return;
+case 0xE0C: /* PMREBS */
+NVME_GUEST_ERR(nvme_ub_mmiowr_pmrebs_readonly,
+   "invalid write to PMREBS register, ignored");
+return;
+case 0xE10: /* PMRSWTP */
+NVME_GUEST_ERR(nvme_ub_mmiowr_pmrswtp_readonly,
+   "invalid write to PMRSWTP register, ignored");
+return;
+case 0xE14: /* TODO PMRMSC */
+ break;
 default:
 NVME_GUEST_ERR(nvme_ub_mmiowr_invalid,
"invalid MMIO write,"
@@ -1169,6 +1200,16 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr 
addr, unsigned size)
 }
 
 if (addr < sizeof(n->bar)) {
+/*
+ 

Re: [PATCH RESEND v4] nvme: introduce PMR support from NVMe 1.4 spec

[Keith Busch]

On Mon, Mar 30, 2020 at 09:46:56AM -0700, Andrzej Jakowski wrote:
> This patch introduces support for PMR that has been defined as part of NVMe 
> 1.4
> spec. User can now specify a pmrdev option that should point to 
> HostMemoryBackend.
> pmrdev memory region will subsequently be exposed as PCI BAR 2 in emulated 
> NVMe
> device. Guest OS can perform mmio read and writes to the PMR region that will 
> stay
> persistent across system reboot.

Looks pretty good to me.

Reviewed-by: Keith Busch 

For a possible future extention, it could be interesting to select the
BAR for PMR dynamically, so that you could have CMB and PMR enabled on
the same device.

Re: [PATCH v9 13/14] iotests: Mark verify functions as private

[Kevin Wolf]

Am 25.03.2020 um 00:21 hat John Snow geschrieben:
> Mark the verify functions as "private" with a leading underscore, to
> discourage their use.
> 
> (Also, make pending patches not yet using the new entry points fail in a
> very obvious way.)
> 
> Signed-off-by: John Snow 
> Reviewed-by: Max Reitz 

While you touch all the function signatures, would you mind adding type
hints? Without them, the hints in execute_setup_common() stay mostly
unused because they can't be checked against callers nor callees.

Kevin

Re: [PATCH v9 10/14] iotests: add hmp helper with logging

[Kevin Wolf]

Am 25.03.2020 um 00:20 hat John Snow geschrieben:
> Just a mild cleanup while I was here.
> 
> Although we now have universal qmp logging on or off, many existing
> callers to hmp functions don't expect that output to be logged, which
> causes quite a few changes in the test output.
> 
> For now, just offer a use_log parameter.
> 
> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/iotests.py | 30 +-
>  1 file changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index e12d6e533e..4faee06f14 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -540,25 +540,29 @@ def add_incoming(self, addr):
>  self._args.append(addr)
>  return self
>  
> -def pause_drive(self, drive, event=None):
> -'''Pause drive r/w operations'''
> +def hmp(self, command_line: str, use_log: bool = False):

Missing return type. Should probably be Dict[str, Any]?

> +cmd = 'human-monitor-command'
> +kwargs = {'command-line': command_line}
> +if use_log:
> +return self.qmp_log(cmd, **kwargs)
> +else:
> +return self.qmp(cmd, **kwargs)
> +
> +def pause_drive(self, drive: str, event: Optional[str] = None) -> None:
> +"""Pause drive r/w operations"""
>  if not event:
>  self.pause_drive(drive, "read_aio")
>  self.pause_drive(drive, "write_aio")
>  return
> -self.qmp('human-monitor-command',
> - command_line='qemu-io %s "break %s bp_%s"'
> - % (drive, event, drive))
> +self.hmp(f'qemu-io {drive} "break {event} bp_{drive}"')
>  
> -def resume_drive(self, drive):
> -self.qmp('human-monitor-command',
> - command_line='qemu-io %s "remove_break bp_%s"'
> - % (drive, drive))
> +def resume_drive(self, drive: str) -> None:
> +"""Resume drive r/w operations"""
> +self.hmp(f'qemu-io {drive} "remove_break bp_{drive}"')
>  
> -def hmp_qemu_io(self, drive, cmd):
> -'''Write to a given drive using an HMP command'''
> -return self.qmp('human-monitor-command',
> -command_line='qemu-io %s "%s"' % (drive, cmd))
> +def hmp_qemu_io(self, drive: str, cmd: str, use_log: bool = False) -> 
> None:
> +"""Write to a given drive using an HMP command"""
> +return self.hmp(f'qemu-io {drive} "{cmd}"', use_log=use_log)

Once you have a non-Any return type for hmp(), this would report that
you return something for a function declared to return None.

Kevin

Re: [PATCH v9 08/14] iotests: touch up log function signature

[Kevin Wolf]

Am 25.03.2020 um 00:20 hat John Snow geschrieben:
> Representing nested, recursive data structures in mypy is notoriously
> difficult; the best we can reliably do right now is denote the atom
> types as "Any" while describing the general shape of the data.
> 
> Regardless, this fully annotates the log() function.
> 
> Typing notes:
> 
> TypeVar is a Type variable that can optionally be constrained by a
> sequence of possible types. This variable is bound per-invocation such
> that the signature for filter=() requires that its callables take e.g. a
> str and return a str.
> 
> Signed-off-by: John Snow 

I like it. Does your version of mypy accept this? I actually get a
warning that doesn't make sense to me:

iotests.py:392: error: Argument 1 to "info" of "Logger" has incompatible 
type "Dict[str, Any]"; expected "str"

The code looks like this:

if isinstance(msg, (dict, list)):
# Don't sort if it's already sorted
do_sort = not isinstance(msg, OrderedDict)
test_logger.info(json.dumps(msg, sort_keys=do_sort, indent=indent))
else:
test_logger.info(msg)

I have no idea why it would think it can still be Dict[str, Any] in the
else branch. Even after adding an 'assert not instanceof(msg, dict), it
still thinks so.

Probably time to update for me...

Kevin

Re: [PATCH 3/6] dump/win_dump: fix use after free of err

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy  writes:

> It's possible that we'll try to set err twice (or more). It's bad, it
> will crash.

True.

> Instead, use warn_report().

Improvement even without the potential crash enabled by the loop.

> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  dump/win_dump.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/dump/win_dump.c b/dump/win_dump.c
> index eda2a48974..652c7bad99 100644
> --- a/dump/win_dump.c
> +++ b/dump/win_dump.c
> @@ -304,13 +304,11 @@ static void restore_context(WinDumpHeader64 *h,
>  struct saved_context *saved_ctx)
>  {
>  int i;
> -Error *err = NULL;
>  
>  for (i = 0; i < h->NumberProcessors; i++) {
>  if (cpu_memory_rw_debug(first_cpu, saved_ctx[i].addr,
>  (uint8_t *)_ctx[i].ctx, sizeof(WinContext), 1)) {
> -error_setg(, "win-dump: failed to restore CPU #%d context", 
> i);
> -warn_report_err(err);
> +warn_report("win-dump: failed to restore CPU #%d context", i);
>  }
>  }
>  }

Reviewed-by: Markus Armbruster 

Re: [PATCH v9 05/14] iotests: add pylintrc file

[Kevin Wolf]

Am 25.03.2020 um 00:20 hat John Snow geschrieben:
> This allows others to get repeatable results with pylint. If you run
> `pylint iotests.py`, you should see a 100% pass.
> 
> Signed-off-by: John Snow 
> Reviewed-by: Max Reitz 

I see you said "should" and not "will", but I still get two warnings:

iotests.py:405:22: W0613: Unused argument 'signum' (unused-argument)
iotests.py:405:30: W0613: Unused argument 'frame' (unused-argument)

Kevin

Re: [PATCH v9 01/14] iotests: do a light delinting

[Kevin Wolf]

Am 25.03.2020 um 00:20 hat John Snow geschrieben:
> This doesn't fix everything in here, but it does help clean up the
> pylint report considerably.
> 
> This should be 100% style changes only; the intent is to make pylint
> more useful by working on establishing a baseline for iotests that we
> can gate against in the future.
> 
> Signed-off-by: John Snow 
> Reviewed-by: Philippe Mathieu-Daudé 
> Reviewed-by: Max Reitz 

> @@ -550,8 +546,8 @@ def flatten_qmp_object(self, obj, output=None, 
> basestr=''):
>  if output is None:
>  output = dict()
>  if isinstance(obj, list):
> -for i in range(len(obj)):
> -self.flatten_qmp_object(obj[i], output, basestr + str(i) + 
> '.')
> +for i, atom in enumerate(obj):
> +self.flatten_qmp_object(atom, output, basestr + str(i) + '.')

I think atom isn't strictly the right word because we expect nested data
structures (as shown by the recursive call). If I understand correctly,
what Python calls things in lists is "items".

Kevin

Re: [PATCH v9 1/4] qcow2: introduce compression type feature

[Denis Plotnikov]

On 30.03.2020 18:06, Markus Armbruster wrote:

Denis Plotnikov  writes:


The patch adds some preparation parts for incompatible compression type
feature to qcow2 allowing the use different compression methods for
image clusters (de)compressing.

It is implied that the compression type is set on the image creation and
can be changed only later by image conversion, thus compression type
defines the only compression algorithm used for the image, and thus,
for all image clusters.

The goal of the feature is to add support of other compression methods
to qcow2. For example, ZSTD which is more effective on compression than ZLIB.

The default compression is ZLIB. Images created with ZLIB compression type
are backward compatible with older qemu versions.

Adding of the compression type breaks a number of tests because now the
compression type is reported on image creation and there are some changes
in the qcow2 header in size and offsets.

The tests are fixed in the following ways:
 * filter out compression_type for many tests
 * fix header size, feature table size and backing file offset
   affected tests: 031, 036, 061, 080
   header_size +=8: 1 byte compression type
7 bytes padding
   feature_table += 48: incompatible feature compression type
   backing_file_offset += 56 (8 + 48 -> header_change + 
feature_table_change)
 * add "compression type" for test output matching when it isn't filtered
   affected tests: 049, 060, 061, 065, 144, 182, 242, 255

Signed-off-by: Denis Plotnikov 

Hmm, haven't I've seen this before?  ...  Yes, I provided my QAPI part:
Acked-by for v5, and the QAPI part hasn't changed since.  You can save
me a bit of labor by carrying my Acked-by forward.

For sure, next time. Thanks!


Once again, QAPI part:
Acked-by: Markus Armbruster 

Re: [PATCH v9 1/4] qcow2: introduce compression type feature

[Markus Armbruster]

Denis Plotnikov  writes:

> The patch adds some preparation parts for incompatible compression type
> feature to qcow2 allowing the use different compression methods for
> image clusters (de)compressing.
>
> It is implied that the compression type is set on the image creation and
> can be changed only later by image conversion, thus compression type
> defines the only compression algorithm used for the image, and thus,
> for all image clusters.
>
> The goal of the feature is to add support of other compression methods
> to qcow2. For example, ZSTD which is more effective on compression than ZLIB.
>
> The default compression is ZLIB. Images created with ZLIB compression type
> are backward compatible with older qemu versions.
>
> Adding of the compression type breaks a number of tests because now the
> compression type is reported on image creation and there are some changes
> in the qcow2 header in size and offsets.
>
> The tests are fixed in the following ways:
> * filter out compression_type for many tests
> * fix header size, feature table size and backing file offset
>   affected tests: 031, 036, 061, 080
>   header_size +=8: 1 byte compression type
>7 bytes padding
>   feature_table += 48: incompatible feature compression type
>   backing_file_offset += 56 (8 + 48 -> header_change + 
> feature_table_change)
> * add "compression type" for test output matching when it isn't filtered
>   affected tests: 049, 060, 061, 065, 144, 182, 242, 255
>
> Signed-off-by: Denis Plotnikov 

Hmm, haven't I've seen this before?  ...  Yes, I provided my QAPI part:
Acked-by for v5, and the QAPI part hasn't changed since.  You can save
me a bit of labor by carrying my Acked-by forward.

Once again, QAPI part:
Acked-by: Markus Armbruster 

Re: [PATCH v11 3/4] qcow2: add zstd cluster compression

[Denis Plotnikov]

On 30.03.2020 16:14, Vladimir Sementsov-Ogievskiy wrote:

30.03.2020 12:54, Denis Plotnikov wrote:

zstd significantly reduces cluster compression time.
It provides better compression performance maintaining
the same level of the compression ratio in comparison with
zlib, which, at the moment, is the only compression
method available.

The performance test results:
Test compresses and decompresses qemu qcow2 image with just
installed rhel-7.6 guest.
Image cluster size: 64K. Image on disk size: 2.2G

The test was conducted with brd disk to reduce the influence
of disk subsystem to the test results.
The results is given in seconds.

compress cmd:
   time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
   src.img [zlib|zstd]_compressed.img
decompress cmd
   time ./qemu-img convert -O qcow2
   [zlib|zstd]_compressed.img uncompressed.img

    compression   decompression
  zlib   zstd   zlib zstd

real 65.5   16.3 (-75 %)    1.9  1.6 (-16 %)
user 65.0   15.8    5.3  2.5
sys   3.3    0.2    2.0  2.0

Both ZLIB and ZSTD gave the same compression ratio: 1.57
compressed image size in both cases: 1.4G

Signed-off-by: Denis Plotnikov 
QAPI part:
Acked-by: Markus Armbruster 
---
  docs/interop/qcow2.txt |   1 +
  configure  |   2 +-
  qapi/block-core.json   |   3 +-
  block/qcow2-threads.c  | 138 +
  block/qcow2.c  |   7 +++
  5 files changed, 149 insertions(+), 2 deletions(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 5597e24474..795dbb21dd 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -208,6 +208,7 @@ version 2.
    Available compression type values:
  0: zlib 
+    1: zstd 
      === Header padding ===
diff --git a/configure b/configure
index da09c35895..57cac2abe1 100755
--- a/configure
+++ b/configure
@@ -1861,7 +1861,7 @@ disabled with --disable-FEATURE, default is 
enabled if available:

    lzfse   support of lzfse compression library
    (for reading lzfse-compressed dmg images)
    zstd    support for zstd compression library
-  (for migration compression)
+  (for migration compression and qcow2 cluster 
compression)

    seccomp seccomp support
    coroutine-pool  coroutine freelist (better performance)
    glusterfs   GlusterFS backend
diff --git a/qapi/block-core.json b/qapi/block-core.json
index d669ec0965..44690ed266 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4293,11 +4293,12 @@
  # Compression type used in qcow2 image file
  #
  # @zlib: zlib compression, see 
+# @zstd: zstd compression, see 
  #
  # Since: 5.0
  ##
  { 'enum': 'Qcow2CompressionType',
-  'data': [ 'zlib' ] }
+  'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } 
] }

    ##
  # @BlockdevCreateOptionsQcow2:
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 7dbaf53489..b8ccd97009 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -28,6 +28,11 @@
  #define ZLIB_CONST
  #include 
  +#ifdef CONFIG_ZSTD
+#include 
+#include 
+#endif
+
  #include "qcow2.h"
  #include "block/thread-pool.h"
  #include "crypto.h"
@@ -166,6 +171,129 @@ static ssize_t qcow2_zlib_decompress(void 
*dest, size_t dest_size,

  return ret;
  }
  +#ifdef CONFIG_ZSTD
+
+/*
+ * qcow2_zstd_compress()
+ *
+ * Compress @src_size bytes of data using zstd compression method
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: compressed size on success
+ *  -ENOMEM destination buffer is not enough to store 
compressed data

+ *  -EIO    on any other error
+ */
+static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
+   const void *src, size_t src_size)
+{
+    size_t ret;
+    ZSTD_outBuffer output = { dest, dest_size, 0 };
+    ZSTD_inBuffer input = { src, src_size, 0 };
+    ZSTD_CCtx *cctx = ZSTD_createCCtx();
+
+    if (!cctx) {
+    return -EIO;
+    }
+    /*
+ * ZSTD spec: "You must continue calling ZSTD_compressStream2()
+ * with ZSTD_e_end until it returns 0, at which point you are
+ * free to start a new frame".
+ * In the loop, we try to compress all the data into one zstd 
frame.

+ * ZSTD_compressStream2 can potentially finish a frame earlier
+ * than the full input data is consumed. That's why we are looping
+ * until all the input data is consumed.
+ */
+    while (input.pos < input.size) {
+    /*
+ * zstd simple interface requires the exact compressed size.


on 

Re: [PATCH v9 05/14] iotests: add pylintrc file

[Markus Armbruster]

John Snow  writes:

> This allows others to get repeatable results with pylint. If you run
> `pylint iotests.py`, you should see a 100% pass.

Nice.

>
> Signed-off-by: John Snow 
> Reviewed-by: Max Reitz 
> ---
>  tests/qemu-iotests/pylintrc | 22 ++
>  1 file changed, 22 insertions(+)
>  create mode 100644 tests/qemu-iotests/pylintrc
>
> diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc
> new file mode 100644
> index 00..8720b6a0de
> --- /dev/null
> +++ b/tests/qemu-iotests/pylintrc
> @@ -0,0 +1,22 @@
> +[MESSAGES CONTROL]
> +
> +# Disable the message, report, category or checker with the given id(s). You
> +# can either give multiple identifiers separated by comma (,) or put this
> +# option multiple times (only on the command line, not in the configuration
> +# file where it should appear only once). You can also use "--disable=all" to
> +# disable everything first and then reenable specific checks. For example, if
> +# you want to run only the similarities checker, you can use "--disable=all
> +# --enable=similarities". If you want to run only the classes checker, but 
> have
> +# no Warning level messages displayed, use "--disable=all --enable=classes
> +# --disable=W".
> +disable=invalid-name,
> +no-else-return,
> +too-many-lines,
> +too-few-public-methods,
> +too-many-arguments,
> +too-many-locals,
> +too-many-branches,
> +too-many-public-methods,

Keep sorted?

> +# These are temporary, and should be removed:
> +missing-docstring,
> +line-too-long,

For what it's worth, I also disable these for checking QAPI code, except
for no-else-return.  My true reason for keeping no-else-return is of
course that I agree with pylint these elses are ugly.  But I pretend to
simply go with the flow ;)

Re: [PATCH v9 01/14] iotests: do a light delinting

[Markus Armbruster]

John Snow  writes:

> This doesn't fix everything in here, but it does help clean up the
> pylint report considerably.
>
> This should be 100% style changes only; the intent is to make pylint
> more useful by working on establishing a baseline for iotests that we
> can gate against in the future.
>
> Signed-off-by: John Snow 
> Reviewed-by: Philippe Mathieu-Daudé 
> Reviewed-by: Max Reitz 
> ---
>  tests/qemu-iotests/iotests.py | 83 ++-
>  1 file changed, 43 insertions(+), 40 deletions(-)
>
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 7bc4934cd2..886ae962ae 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
[...]
> @@ -1062,6 +1063,7 @@ def func_wrapper(test_case: QMPTestCase, *args, 
> **kwargs):
>  if usf_list:
>  test_case.case_skip('{}: formats {} are not 
> whitelisted'.format(
>  test_case, usf_list))
> +return None
>  else:
>  return func(test_case, *args, **kwargs)
>  return func_wrapper
> @@ -1073,6 +1075,7 @@ def skip_if_user_is_root(func):
>  def func_wrapper(*args, **kwargs):
>  if os.getuid() == 0:
>  case_notrun('{}: cannot be run as root'.format(args[0]))
> +return None
>  else:
>  return func(*args, **kwargs)
>  return func_wrapper

Observation, not demand: this trades one kind of pylint report for
another: inconsistent-return-statements for no-else-return.  PATCH 05
suppresses no-else-return.

Re: [PATCH RESEND v3 0/4] virtio-pci: enable blk and scsi multi-queue by default

[Pankaj Gupta]

For best case its really a good idea to configure default number of
queues to the number of CPU's.

For the series:
Reviewed-by: Pankaj Gupta 

[PATCH 3/3] block: use int64_t instead of uint64_t in driver handlers

[Vladimir Sementsov-Ogievskiy]

We are generally moving to int64_t for both offset and bytes parameters
on all io paths. Convert driver handlers parameters which are already
64bit to signed type.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 include/block/block_int.h | 28 ++--
 block/backup-top.c|  5 ++---
 block/blkdebug.c  |  4 ++--
 block/blklogwrites.c  |  4 ++--
 block/blkreplay.c |  4 ++--
 block/blkverify.c |  4 ++--
 block/bochs.c |  2 +-
 block/cloop.c |  2 +-
 block/commit.c|  2 +-
 block/copy-on-read.c  |  4 ++--
 block/crypto.c|  4 ++--
 block/curl.c  |  2 +-
 block/dmg.c   |  2 +-
 block/file-posix.c| 18 +-
 block/filter-compress.c   |  6 +++---
 block/iscsi.c | 12 ++--
 block/mirror.c|  4 ++--
 block/nbd.c   |  8 
 block/nfs.c   |  8 
 block/null.c  |  8 
 block/nvme.c  |  4 ++--
 block/qcow.c  | 12 ++--
 block/qcow2.c | 18 +-
 block/quorum.c|  8 
 block/raw-format.c| 28 ++--
 block/rbd.c   |  4 ++--
 block/throttle.c  |  4 ++--
 block/vdi.c   |  4 ++--
 block/vmdk.c  |  8 
 block/vpc.c   |  4 ++--
 block/vvfat.c |  6 +++---
 tests/test-bdrv-drain.c   |  8 
 block/trace-events|  2 +-
 33 files changed, 120 insertions(+), 121 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 28aea2bcfd..ea8fca5e28 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -180,10 +180,10 @@ struct BlockDriver {
 
 /* aio */
 BlockAIOCB *(*bdrv_aio_preadv)(BlockDriverState *bs,
-uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags,
+int64_t offset, int64_t bytes, QEMUIOVector *qiov, int flags,
 BlockCompletionFunc *cb, void *opaque);
 BlockAIOCB *(*bdrv_aio_pwritev)(BlockDriverState *bs,
-uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags,
+int64_t offset, int64_t bytes, QEMUIOVector *qiov, int flags,
 BlockCompletionFunc *cb, void *opaque);
 BlockAIOCB *(*bdrv_aio_flush)(BlockDriverState *bs,
 BlockCompletionFunc *cb, void *opaque);
@@ -210,9 +210,9 @@ struct BlockDriver {
  * The buffer in @qiov may point directly to guest memory.
  */
 int coroutine_fn (*bdrv_co_preadv)(BlockDriverState *bs,
-uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags);
+int64_t offset, int64_t bytes, QEMUIOVector *qiov, int flags);
 int coroutine_fn (*bdrv_co_preadv_part)(BlockDriverState *bs,
-uint64_t offset, uint64_t bytes,
+int64_t offset, int64_t bytes,
 QEMUIOVector *qiov, size_t qiov_offset, int flags);
 int coroutine_fn (*bdrv_co_writev)(BlockDriverState *bs,
 int64_t sector_num, int nb_sectors, QEMUIOVector *qiov, int flags);
@@ -232,9 +232,9 @@ struct BlockDriver {
  * The buffer in @qiov may point directly to guest memory.
  */
 int coroutine_fn (*bdrv_co_pwritev)(BlockDriverState *bs,
-uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags);
+int64_t offset, int64_t bytes, QEMUIOVector *qiov, int flags);
 int coroutine_fn (*bdrv_co_pwritev_part)(BlockDriverState *bs,
-uint64_t offset, uint64_t bytes,
+int64_t offset, int64_t bytes,
 QEMUIOVector *qiov, size_t qiov_offset, int flags);
 
 /*
@@ -257,10 +257,10 @@ struct BlockDriver {
  */
 int coroutine_fn (*bdrv_co_copy_range_from)(BlockDriverState *bs,
 BdrvChild *src,
-uint64_t offset,
+int64_t offset,
 BdrvChild *dst,
-uint64_t dst_offset,
-uint64_t bytes,
+int64_t dst_offset,
+int64_t bytes,
 BdrvRequestFlags read_flags,
 BdrvRequestFlags write_flags);
 
@@ -274,10 +274,10 @@ struct BlockDriver {
  */
 int coroutine_fn (*bdrv_co_copy_range_to)(BlockDriverState *bs,
   BdrvChild *src,
-  uint64_t src_offset,
+  int64_t src_offset,
   BdrvChild *dst,
-  uint64_t dst_offset,
-  uint64_t bytes,
+  

[PATCH 2/3] block/io: convert generic io path to use int64_t parameters

[Vladimir Sementsov-Ogievskiy]

We are generally moving to int64_t for both offset and bytes paramaters
on all io paths. Convert generic io path now.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 include/block/block.h |  8 ++--
 include/block/block_int.h | 20 -
 block/blkverify.c |  2 +-
 block/io.c| 86 +++
 block/trace-events| 12 +++---
 5 files changed, 64 insertions(+), 64 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index b05995fe9c..fa5c7285b6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -333,7 +333,7 @@ int bdrv_pwrite_sync(BdrvChild *child, int64_t offset,
  * because it may allocate memory for the entire region.
  */
 int coroutine_fn bdrv_co_pwrite_zeroes(BdrvChild *child, int64_t offset,
-   int bytes, BdrvRequestFlags flags);
+   int64_t bytes, BdrvRequestFlags flags);
 BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
 const char *backing_file);
 void bdrv_refresh_filename(BlockDriverState *bs);
@@ -731,8 +731,8 @@ void bdrv_unregister_buf(BlockDriverState *bs, void *host);
  *
  * Returns: 0 if succeeded; negative error code if failed.
  **/
-int coroutine_fn bdrv_co_copy_range(BdrvChild *src, uint64_t src_offset,
-BdrvChild *dst, uint64_t dst_offset,
-uint64_t bytes, BdrvRequestFlags 
read_flags,
+int coroutine_fn bdrv_co_copy_range(BdrvChild *src, int64_t src_offset,
+BdrvChild *dst, int64_t dst_offset,
+int64_t bytes, BdrvRequestFlags read_flags,
 BdrvRequestFlags write_flags);
 #endif
diff --git a/include/block/block_int.h b/include/block/block_int.h
index c8daba608b..28aea2bcfd 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -972,16 +972,16 @@ extern BlockDriver bdrv_raw;
 extern BlockDriver bdrv_qcow2;
 
 int coroutine_fn bdrv_co_preadv(BdrvChild *child,
-int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
+int64_t offset, int64_t bytes, QEMUIOVector *qiov,
 BdrvRequestFlags flags);
 int coroutine_fn bdrv_co_preadv_part(BdrvChild *child,
-int64_t offset, unsigned int bytes,
+int64_t offset, int64_t bytes,
 QEMUIOVector *qiov, size_t qiov_offset, BdrvRequestFlags flags);
 int coroutine_fn bdrv_co_pwritev(BdrvChild *child,
-int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
+int64_t offset, int64_t bytes, QEMUIOVector *qiov,
 BdrvRequestFlags flags);
 int coroutine_fn bdrv_co_pwritev_part(BdrvChild *child,
-int64_t offset, unsigned int bytes,
+int64_t offset, int64_t bytes,
 QEMUIOVector *qiov, size_t qiov_offset, BdrvRequestFlags flags);
 
 static inline int coroutine_fn bdrv_co_pread(BdrvChild *child,
@@ -1315,14 +1315,14 @@ void bdrv_dec_in_flight(BlockDriverState *bs);
 
 void blockdev_close_all_bdrv_states(void);
 
-int coroutine_fn bdrv_co_copy_range_from(BdrvChild *src, uint64_t src_offset,
- BdrvChild *dst, uint64_t dst_offset,
- uint64_t bytes,
+int coroutine_fn bdrv_co_copy_range_from(BdrvChild *src, int64_t src_offset,
+ BdrvChild *dst, int64_t dst_offset,
+ int64_t bytes,
  BdrvRequestFlags read_flags,
  BdrvRequestFlags write_flags);
-int coroutine_fn bdrv_co_copy_range_to(BdrvChild *src, uint64_t src_offset,
-   BdrvChild *dst, uint64_t dst_offset,
-   uint64_t bytes,
+int coroutine_fn bdrv_co_copy_range_to(BdrvChild *src, int64_t src_offset,
+   BdrvChild *dst, int64_t dst_offset,
+   int64_t bytes,
BdrvRequestFlags read_flags,
BdrvRequestFlags write_flags);
 
diff --git a/block/blkverify.c b/block/blkverify.c
index ba6b1853ae..667e60d832 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -31,7 +31,7 @@ typedef struct BlkverifyRequest {
 uint64_t bytes;
 int flags;
 
-int (*request_fn)(BdrvChild *, int64_t, unsigned int, QEMUIOVector *,
+int (*request_fn)(BdrvChild *, int64_t, int64_t, QEMUIOVector *,
   BdrvRequestFlags);
 
 int ret;/* test image result */
diff --git a/block/io.c b/block/io.c
index 7cbb80bd24..79e3014489 100644
--- a/block/io.c
+++ b/block/io.c
@@ -42,7 +42,7 @@
 
 static void bdrv_parent_cb_resize(BlockDriverState *bs);
 static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
-int64_t offset, int bytes, BdrvRequestFlags flags);
+int64_t offset, int64_t bytes, 

[RFC 0/3] 64bit block-layer part I

[Vladimir Sementsov-Ogievskiy]

Hi all!

There is an idea to make NBD protocol extension to support 64bit
write-zero/discard/block-status commands (commands, which doesn't
transfer user data). It's needed to increase performance of zeroing
large ranges (up to the whole image). Zeroing of the whole image is used
as first step of mirror job, qemu-img convert, it should be also used at
start of backup actually..

We need to support it in block-layer, so we want 64bit write_zeros.
Currently driver handler now have int bytes parameter.

write_zeros path goes through normal pwritev, so we need 64bit write,
and then we need 64bit read for symmetry, and better, let's make all io
path work with 64bit bytes parameter.

Actually most of block-layer already have 64bit parameters: offset is
sometimes int64_t and sometimes uint64_t. bytes parameter is one of
int64_t, uint64_t, int, unsigned int...

I think we need one type for all of this, and this one type is int64_t.
Signed int64_t is a bit better than uint64_t: you can use same variable
to get some result (including error < 0) and than reuse it as an
argument without any type conversion.

So, I propose, as a first step, convert all uint64_t parameters to
int64_t.

Still, I don't have good idea of how to split this into more than 3
patches, so, this is an RFC.

What's next?

Converting write_zero and discard is not as simple: we can't just
s/int/uint64_t/, as some functions may use some int variables for
calculations and this will be broken by something larger than int.

So, I think the simplest way is to add .bdrv_co_pwritev_zeros64 and
.bdrv_co_pdiscard64 and update drivers one-by-one. If at some point all
drivers updated - drop unused 32bit functions, and then drop "64" suffix
from API. If not - we'll live with both APIs.

Another thing to do is updating default limiting of request (currently
they are limited to INT_MAX).

Then we may move some drivers to 64bit discard/write_zero: I think about
qcow2, file-posix and nbd (as a proof-of-concept for already proposed
NBD extension).

Any ideas?

Vladimir Sementsov-Ogievskiy (3):
  block: use int64_t as bytes type in tracked requests
  block/io: convert generic io path to use int64_t parameters
  block: use int64_t instead of uint64_t in driver handlers

 include/block/block.h |  8 ++--
 include/block/block_int.h | 52 ++---
 block/backup-top.c|  5 +-
 block/blkdebug.c  |  4 +-
 block/blklogwrites.c  |  4 +-
 block/blkreplay.c |  4 +-
 block/blkverify.c |  6 +--
 block/bochs.c |  2 +-
 block/cloop.c |  2 +-
 block/commit.c|  2 +-
 block/copy-on-read.c  |  4 +-
 block/crypto.c|  4 +-
 block/curl.c  |  2 +-
 block/dmg.c   |  2 +-
 block/file-posix.c| 18 
 block/filter-compress.c   |  6 +--
 block/io.c| 97 ---
 block/iscsi.c | 12 ++---
 block/mirror.c|  4 +-
 block/nbd.c   |  8 ++--
 block/nfs.c   |  8 ++--
 block/null.c  |  8 ++--
 block/nvme.c  |  4 +-
 block/qcow.c  | 12 ++---
 block/qcow2.c | 18 
 block/quorum.c|  8 ++--
 block/raw-format.c| 28 +--
 block/rbd.c   |  4 +-
 block/throttle.c  |  4 +-
 block/vdi.c   |  4 +-
 block/vmdk.c  |  8 ++--
 block/vpc.c   |  4 +-
 block/vvfat.c |  6 +--
 tests/test-bdrv-drain.c   |  8 ++--
 block/trace-events| 14 +++---
 35 files changed, 192 insertions(+), 192 deletions(-)

-- 
2.21.0

[PATCH 1/3] block: use int64_t as bytes type in tracked requests

[Vladimir Sementsov-Ogievskiy]

We are generally moving to int64_t for both offset and bytes paramaters
on all io paths. Convert tracked requests now.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 include/block/block_int.h |  4 ++--
 block/io.c| 11 ++-
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 4c3587ea19..c8daba608b 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -70,12 +70,12 @@ enum BdrvTrackedRequestType {
 typedef struct BdrvTrackedRequest {
 BlockDriverState *bs;
 int64_t offset;
-uint64_t bytes;
+int64_t bytes;
 enum BdrvTrackedRequestType type;
 
 bool serialising;
 int64_t overlap_offset;
-uint64_t overlap_bytes;
+int64_t overlap_bytes;
 
 QLIST_ENTRY(BdrvTrackedRequest) list;
 Coroutine *co; /* owner, used for deadlock detection */
diff --git a/block/io.c b/block/io.c
index aba67f66b9..7cbb80bd24 100644
--- a/block/io.c
+++ b/block/io.c
@@ -692,10 +692,11 @@ static void tracked_request_end(BdrvTrackedRequest *req)
 static void tracked_request_begin(BdrvTrackedRequest *req,
   BlockDriverState *bs,
   int64_t offset,
-  uint64_t bytes,
+  int64_t bytes,
   enum BdrvTrackedRequestType type)
 {
-assert(bytes <= INT64_MAX && offset <= INT64_MAX - bytes);
+assert(offset >= 0 && bytes >= 0 &&
+   bytes <= INT64_MAX && offset <= INT64_MAX - bytes);
 
 *req = (BdrvTrackedRequest){
 .bs = bs,
@@ -716,7 +717,7 @@ static void tracked_request_begin(BdrvTrackedRequest *req,
 }
 
 static bool tracked_request_overlaps(BdrvTrackedRequest *req,
- int64_t offset, uint64_t bytes)
+ int64_t offset, int64_t bytes)
 {
 /*    */
 if (offset >= req->overlap_offset + req->overlap_bytes) {
@@ -773,8 +774,8 @@ bool bdrv_mark_request_serialising(BdrvTrackedRequest *req, 
uint64_t align)
 {
 BlockDriverState *bs = req->bs;
 int64_t overlap_offset = req->offset & ~(align - 1);
-uint64_t overlap_bytes = ROUND_UP(req->offset + req->bytes, align)
-   - overlap_offset;
+int64_t overlap_bytes =
+ROUND_UP(req->offset + req->bytes, align) - overlap_offset;
 bool waited;
 
 qemu_co_mutex_lock(>reqs_lock);
-- 
2.21.0

Re: [PATCH v4 1/2] virtio-blk: delete vqs on the error path in realize()

[Pankaj Gupta]

Reviewed-by: Pankaj Gupta 

Re: [PATCH v11 3/4] qcow2: add zstd cluster compression

[Vladimir Sementsov-Ogievskiy]

30.03.2020 12:54, Denis Plotnikov wrote:

zstd significantly reduces cluster compression time.
It provides better compression performance maintaining
the same level of the compression ratio in comparison with
zlib, which, at the moment, is the only compression
method available.

The performance test results:
Test compresses and decompresses qemu qcow2 image with just
installed rhel-7.6 guest.
Image cluster size: 64K. Image on disk size: 2.2G

The test was conducted with brd disk to reduce the influence
of disk subsystem to the test results.
The results is given in seconds.

compress cmd:
   time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
   src.img [zlib|zstd]_compressed.img
decompress cmd
   time ./qemu-img convert -O qcow2
   [zlib|zstd]_compressed.img uncompressed.img

compression   decompression
  zlib   zstd   zlib zstd

real 65.5   16.3 (-75 %)1.9  1.6 (-16 %)
user 65.0   15.85.3  2.5
sys   3.30.22.0  2.0

Both ZLIB and ZSTD gave the same compression ratio: 1.57
compressed image size in both cases: 1.4G

Signed-off-by: Denis Plotnikov 
QAPI part:
Acked-by: Markus Armbruster 
---
  docs/interop/qcow2.txt |   1 +
  configure  |   2 +-
  qapi/block-core.json   |   3 +-
  block/qcow2-threads.c  | 138 +
  block/qcow2.c  |   7 +++
  5 files changed, 149 insertions(+), 2 deletions(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 5597e24474..795dbb21dd 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -208,6 +208,7 @@ version 2.
  
  Available compression type values:

  0: zlib 
+1: zstd 
  
  
  === Header padding ===

diff --git a/configure b/configure
index da09c35895..57cac2abe1 100755
--- a/configure
+++ b/configure
@@ -1861,7 +1861,7 @@ disabled with --disable-FEATURE, default is enabled if 
available:
lzfse   support of lzfse compression library
(for reading lzfse-compressed dmg images)
zstdsupport for zstd compression library
-  (for migration compression)
+  (for migration compression and qcow2 cluster compression)
seccomp seccomp support
coroutine-pool  coroutine freelist (better performance)
glusterfs   GlusterFS backend
diff --git a/qapi/block-core.json b/qapi/block-core.json
index d669ec0965..44690ed266 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4293,11 +4293,12 @@
  # Compression type used in qcow2 image file
  #
  # @zlib: zlib compression, see 
+# @zstd: zstd compression, see 
  #
  # Since: 5.0
  ##
  { 'enum': 'Qcow2CompressionType',
-  'data': [ 'zlib' ] }
+  'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
  
  ##

  # @BlockdevCreateOptionsQcow2:
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 7dbaf53489..b8ccd97009 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -28,6 +28,11 @@
  #define ZLIB_CONST
  #include 
  
+#ifdef CONFIG_ZSTD

+#include 
+#include 
+#endif
+
  #include "qcow2.h"
  #include "block/thread-pool.h"
  #include "crypto.h"
@@ -166,6 +171,129 @@ static ssize_t qcow2_zlib_decompress(void *dest, size_t 
dest_size,
  return ret;
  }
  
+#ifdef CONFIG_ZSTD

+
+/*
+ * qcow2_zstd_compress()
+ *
+ * Compress @src_size bytes of data using zstd compression method
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: compressed size on success
+ *  -ENOMEM destination buffer is not enough to store compressed data
+ *  -EIOon any other error
+ */
+static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
+   const void *src, size_t src_size)
+{
+size_t ret;
+ZSTD_outBuffer output = { dest, dest_size, 0 };
+ZSTD_inBuffer input = { src, src_size, 0 };
+ZSTD_CCtx *cctx = ZSTD_createCCtx();
+
+if (!cctx) {
+return -EIO;
+}
+/*
+ * ZSTD spec: "You must continue calling ZSTD_compressStream2()
+ * with ZSTD_e_end until it returns 0, at which point you are
+ * free to start a new frame".
+ * In the loop, we try to compress all the data into one zstd frame.
+ * ZSTD_compressStream2 can potentially finish a frame earlier
+ * than the full input data is consumed. That's why we are looping
+ * until all the input data is consumed.
+ */
+while (input.pos < input.size) {
+/*
+ * zstd simple interface requires the exact compressed size.


on decompression you mean


+ * zstd stream interface 

Re: [PATCH v9 00/14] iotests: use python logging

[Max Reitz]

On 25.03.20 00:20, John Snow wrote:
> This series uses python logging to enable output conditionally on
> iotests.log(). We unify an initialization call (which also enables
> debugging output for those tests with -d) and then make the switch
> inside of iotests.
> 
> It will help alleviate the need to create logged/unlogged versions
> of all the various helpers we have made.
> 
> Also, I got lost and accidentally delinted iotests while I was here.
> Sorry about that. By version 9, it's now the overwhelming focus of
> this series. No good deed, etc.

Generally, test patches are fair game for the freeze period.  However,
this series is quite extensive, so I might prefer block-next here.
OTOH, if I do take it to block-next, patch 11 might grow stale.

Do you have a strong opinion either way?

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v9 10/14] iotests: add hmp helper with logging

[Max Reitz]

On 25.03.20 00:20, John Snow wrote:
> Just a mild cleanup while I was here.
> 
> Although we now have universal qmp logging on or off, many existing
> callers to hmp functions don't expect that output to be logged, which
> causes quite a few changes in the test output.
> 
> For now, just offer a use_log parameter.
> 
> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/iotests.py | 30 +-
>  1 file changed, 17 insertions(+), 13 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v9 09/14] iotests: limit line length to 79 chars

[Max Reitz]

On 30.03.20 14:31, Max Reitz wrote:
> On 25.03.20 00:20, John Snow wrote:
>> 79 is the PEP8 recommendation. This recommendation works well for
>> reading patch diffs in TUI email clients.
>>
>> Signed-off-by: John Snow 
>> ---
>>  tests/qemu-iotests/iotests.py | 64 +++
>>  tests/qemu-iotests/pylintrc   |  6 +++-
>>  2 files changed, 47 insertions(+), 23 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 3a049ece5b..e12d6e533e 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
> 
> [...]
> 
>> @@ -537,11 +547,13 @@ def pause_drive(self, drive, event=None):
>>  self.pause_drive(drive, "write_aio")
>>  return
>>  self.qmp('human-monitor-command',
>> - command_line='qemu-io %s "break %s bp_%s"' % (drive, 
>> event, drive))
>> + command_line='qemu-io %s "break %s bp_%s"'
>> + % (drive, event, drive))
>>  
>>  def resume_drive(self, drive):
>>  self.qmp('human-monitor-command',
>> - command_line='qemu-io %s "remove_break bp_%s"' % (drive, 
>> drive))
>> + command_line='qemu-io %s "remove_break bp_%s"'
>> + % (drive, drive))
> 
> Hm.  You didn’t reply on my (second, insisting) question on what stops
> us from introducing a variable here (hmp_cmd = 'qemu-io %s ...' % ...;
> self.qmp('human-monitor-command', command_line=hmp_cmd)).
> 
> :c

Ah!  The next patch is there to address my whining.

c:

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v5 3/4] qmp: Move dispatcher to a coroutine

[Markus Armbruster]

Marc-André Lureau  writes:

> Hi
>
> On Mon, Mar 23, 2020 at 6:41 PM Kevin Wolf  wrote:
>>
>> Am 18.03.2020 um 16:36 hat Markus Armbruster geschrieben:
>> > Kevin Wolf  writes:
>> >
>> > > This moves the QMP dispatcher to a coroutine and runs all QMP command
>> > > handlers that declare 'coroutine': true in coroutine context so they
>> > > can avoid blocking the main loop while doing I/O or waiting for other
>> > > events.
>> > >
>> > > For commands that are not declared safe to run in a coroutine, the
>> > > dispatcher drops out of coroutine context by calling the QMP command
>> > > handler from a bottom half.
>> > >
>> > > Signed-off-by: Kevin Wolf 
>> >
>> > Uh, what about @cur_mon?
>> >
>> > @cur_mon points to the current monitor while a command executes.
>> > Initial value is null.  It is set in three places (not counting unit
>> > tests), and all three save, set, do something that may use @cur_mon,
>> > restore:
>> >
>> > * monitor_qmp_dispatch(), for use within qmp_dispatch()
>> > * monitor_read(), for use within handle_hmp_command()
>> > * qmp_human_monitor_command(), also for use within handle_hmp_command()
>> >
>> > Therefore, @cur_mon is null unless we're running within qmp_dispatch()
>> > or handle_hmp_command().
>>
>> Can we make it NULL for coroutine-enabled handlers?
>
> fwiw, I think /dev/fdset doesn't care about cur_mon. However, qmp
> handlers that use monitor_get_fd() usually depend on cur_mon.
>
> Note: I wonder if add-fd (fdsets) and getfd (monitor fds) deserve to co-exist.

Beats me.

If one of them is more general, we could consider deprecating the other
one.

Re: [PATCH v9 09/14] iotests: limit line length to 79 chars

[Max Reitz]

On 25.03.20 00:20, John Snow wrote:
> 79 is the PEP8 recommendation. This recommendation works well for
> reading patch diffs in TUI email clients.
> 
> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/iotests.py | 64 +++
>  tests/qemu-iotests/pylintrc   |  6 +++-
>  2 files changed, 47 insertions(+), 23 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 3a049ece5b..e12d6e533e 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py

[...]

> @@ -537,11 +547,13 @@ def pause_drive(self, drive, event=None):
>  self.pause_drive(drive, "write_aio")
>  return
>  self.qmp('human-monitor-command',
> - command_line='qemu-io %s "break %s bp_%s"' % (drive, event, 
> drive))
> + command_line='qemu-io %s "break %s bp_%s"'
> + % (drive, event, drive))
>  
>  def resume_drive(self, drive):
>  self.qmp('human-monitor-command',
> - command_line='qemu-io %s "remove_break bp_%s"' % (drive, 
> drive))
> + command_line='qemu-io %s "remove_break bp_%s"'
> + % (drive, drive))

Hm.  You didn’t reply on my (second, insisting) question on what stops
us from introducing a variable here (hmp_cmd = 'qemu-io %s ...' % ...;
self.qmp('human-monitor-command', command_line=hmp_cmd)).

:c

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v5 3/4] qmp: Move dispatcher to a coroutine

[Markus Armbruster]

Kevin Wolf  writes:

> Am 18.03.2020 um 16:36 hat Markus Armbruster geschrieben:
>> Kevin Wolf  writes:
>> 
>> > This moves the QMP dispatcher to a coroutine and runs all QMP command
>> > handlers that declare 'coroutine': true in coroutine context so they
>> > can avoid blocking the main loop while doing I/O or waiting for other
>> > events.
>> >
>> > For commands that are not declared safe to run in a coroutine, the
>> > dispatcher drops out of coroutine context by calling the QMP command
>> > handler from a bottom half.
>> >
>> > Signed-off-by: Kevin Wolf 
>> 
>> Uh, what about @cur_mon?
>> 
>> @cur_mon points to the current monitor while a command executes.
>> Initial value is null.  It is set in three places (not counting unit
>> tests), and all three save, set, do something that may use @cur_mon,
>> restore:
>> 
>> * monitor_qmp_dispatch(), for use within qmp_dispatch()
>> * monitor_read(), for use within handle_hmp_command()
>> * qmp_human_monitor_command(), also for use within handle_hmp_command()
>> 
>> Therefore, @cur_mon is null unless we're running within qmp_dispatch()
>> or handle_hmp_command().
>
> Can we make it NULL for coroutine-enabled handlers?

Sets up and arms a bear trap, I'm afraid.

We can rely on code review to ensure a handler we want to
coroutine-enable does not access @cur_mon.  But how can we ensure no
uses creep back?  Not even on error paths, where testing tends to be
ineffective?

@cur_mon goes back to Jan's commit 376253ece48 (March 2009).  Meant to
be merely a stop gap:

For the case that monitor outputs so far happen without clearly
identifiable context, the global variable cur_mon is introduced that
shall once provide a pointer either to the current active monitor (while
processing commands) or to the default one. On the mid or long term,
those use case will be obsoleted so that this variable can be removed
again.

It's been eleven years, and we haven't really gotten closer to getting
them "obsoleted".  I can't stop you if you want to try.  Myself, I'd
rather give up and eliminate the Monitor * parameter passing.

>> Example of use: error_report() & friends print "to current monitor if we
>> have one, else to stderr."  Makes sharing code between HMP and CLI
>> easier.  Uses @cur_mon under the hood.
>
> error_report() eventually prints to stderr both for cur_mon == NULL and
> for QMP monitors. Is there an important difference between both cases?

This goes back to

commit 4ad417baa43424b6b988c52b83989fd95670c113
Author: Cole Robinson 
Date:   Fri Mar 21 19:42:24 2014 -0400

error: Print error_report() to stderr if using qmp

monitor_printf will drop the requested output if cur_mon is qmp (for
good reason). However these messages are often helpful for debugging
issues with via libvirt.

If we know the message won't hit the monitor, send it to stderr.

Cc: Luiz Capitulino 
Cc: Markus Armbruster 
Signed-off-by: Cole Robinson 
Reviewed-by: Paolo Bonzini 
Signed-off-by: Luiz Capitulino 

Use of error_report() in QMP context is wrong more often than not.
Still, spewing the message to stderr is less bad than throwing it away.

> There is error_vprintf_unless_qmp(), which behaves differently for both
> cases. However, it is only used in VNC code, so that code would have to
> keep coroutines disabled.

It has seen other users over the years.

error_printf_unless_qmp() is a convenient way to print hints for humans.
In HMP context, you reach the human by printing to @cur_mon.  Outside
monitor context, you reach the human by printing to stdout or stderr.
In QMP context, we're talking to a machine.

> Is cur_mon used much in other functions called by QMP handlers?

Marc-André pointed to the file-descriptor passing stuff.

To be sure that's all, we'd have to review all uses of @cur_mon.

>> @cur_mon is thread-local.
>> 
>> I'm afraid we have to save, clear and restore @cur_mon around a yield.
>
> That sounds painful enough that I'd rather avoid it.

I've seen coroutine implementations for C that provide coroutine-local
variables of sorts by managing a thread-local opaque pointer.

Re: [PATCH v9 08/14] iotests: touch up log function signature

[Max Reitz]

On 25.03.20 00:20, John Snow wrote:
> Representing nested, recursive data structures in mypy is notoriously
> difficult; the best we can reliably do right now is denote the atom
> types as "Any" while describing the general shape of the data.
> 
> Regardless, this fully annotates the log() function.
> 
> Typing notes:
> 
> TypeVar is a Type variable that can optionally be constrained by a
> sequence of possible types. This variable is bound per-invocation such
> that the signature for filter=() requires that its callables take e.g. a
> str and return a str.

So it works like a generic, except that its declaration isn’t part of
and thus local to the function.  Interesting.  Not sure if I like it,
but, well, it’s Python.  (Seems to me on the same level of “Interesting”
as the “Don’t use an empty list as a default argument” thing.[1])

> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/iotests.py | 14 +++---
>  1 file changed, 11 insertions(+), 3 deletions(-)

Haha.  I can’t help but find it funny how my “I won’t complain, but I
feel like some people might want this to be two patches” led to this “If
I’m going to make a dedicated patch for this, I might as well go all the
way” patch.  It’s quite cool.

Reviewed-by: Max Reitz 

[1] I just read an article on that thing again (empty lists as default
argument), which claims that this is in fact a very useful feature of
Python.  This makes me want to go on another rant, but I’ve learned
enough restraint by now not to.



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v9 07/14] iotests: drop pre-Python 3.4 compatibility code

[Max Reitz]

On 25.03.20 00:20, John Snow wrote:
> We no longer need to accommodate 3.4, drop this code.
> (The lines were > 79 chars and it stood out.)
> 
> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/iotests.py | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
I did say I wouldn’t complain about the unrelated change!

*shrug*

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v9 06/14] iotests: alphabetize standard imports

[Max Reitz]

On 25.03.20 00:20, John Snow wrote:
> I had to fix a merge conflict, so do this tiny harmless thing while I'm
> here.
> 
> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/iotests.py | 18 +-
>  1 file changed, 9 insertions(+), 9 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v4] qcow2: Forbid discard in qcow2 v2 images with backing files

[Max Reitz]

On 27.03.20 19:59, Alberto Garcia wrote:
> A discard request deallocates the selected clusters so they read back
> as zeroes. This is done by clearing the cluster offset field and
> setting QCOW_OFLAG_ZERO in the L2 entry.
> 
> This flag is however only supported when qcow_version >= 3. In older
> images the cluster is simply deallocated, exposing any possible stale
> data from the backing file.
> 
> Since discard is an advisory operation it's safer to simply forbid it
> in this scenario.
> 
> Note that we are adding this check to qcow2_co_pdiscard() and not to
> qcow2_cluster_discard() or discard_in_l2_slice() because the last
> two are also used by qcow2_snapshot_create() to discard the clusters
> used by the VM state. In this case there's no risk of exposing stale
> data to the guest and we really want that the clusters are always
> discarded.
> 
> Signed-off-by: Alberto Garcia 
> ---
> v4:
> - Show output of qemu-img map when there's no backing file [Eric]
> 
> v3:
> - Rebase and change iotest number
> - Show output of qemu-img map in iotest 290 [Kevin]
> - Use the l2_offset and rb_offset variables in iotest 060
> 
> v2:
> 
> - Don't create the image with compat=0.10 in iotest 060 [Max]
> - Use $TEST_IMG.base for the backing image name in iotest 289 [Max]
> - Add list of unsupported options to iotest 289 [Max]
> 
>  block/qcow2.c  |  6 +++
>  tests/qemu-iotests/060 | 12 ++---
>  tests/qemu-iotests/060.out |  2 -
>  tests/qemu-iotests/290 | 97 ++
>  tests/qemu-iotests/290.out | 61 
>  tests/qemu-iotests/group   |  1 +
>  6 files changed, 170 insertions(+), 9 deletions(-)
>  create mode 100755 tests/qemu-iotests/290
>  create mode 100644 tests/qemu-iotests/290.out

Thanks, applied to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max



signature.asc
Description: OpenPGP digital signature

[PATCH v11 2/4] qcow2: rework the cluster compression routine

[Denis Plotnikov]

The patch enables processing the image compression type defined
for the image and chooses an appropriate method for image clusters
(de)compression.

Signed-off-by: Denis Plotnikov 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Alberto Garcia 
---
 block/qcow2-threads.c | 71 ---
 1 file changed, 60 insertions(+), 11 deletions(-)

diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index a68126f291..7dbaf53489 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -74,7 +74,9 @@ typedef struct Qcow2CompressData {
 } Qcow2CompressData;
 
 /*
- * qcow2_compress()
+ * qcow2_zlib_compress()
+ *
+ * Compress @src_size bytes of data using zlib compression method
  *
  * @dest - destination buffer, @dest_size bytes
  * @src - source buffer, @src_size bytes
@@ -83,8 +85,8 @@ typedef struct Qcow2CompressData {
  *  -ENOMEM destination buffer is not enough to store compressed data
  *  -EIOon any other error
  */
-static ssize_t qcow2_compress(void *dest, size_t dest_size,
-  const void *src, size_t src_size)
+static ssize_t qcow2_zlib_compress(void *dest, size_t dest_size,
+   const void *src, size_t src_size)
 {
 ssize_t ret;
 z_stream strm;
@@ -119,10 +121,10 @@ static ssize_t qcow2_compress(void *dest, size_t 
dest_size,
 }
 
 /*
- * qcow2_decompress()
+ * qcow2_zlib_decompress()
  *
  * Decompress some data (not more than @src_size bytes) to produce exactly
- * @dest_size bytes.
+ * @dest_size bytes using zlib compression method
  *
  * @dest - destination buffer, @dest_size bytes
  * @src - source buffer, @src_size bytes
@@ -130,8 +132,8 @@ static ssize_t qcow2_compress(void *dest, size_t dest_size,
  * Returns: 0 on success
  *  -EIO on fail
  */
-static ssize_t qcow2_decompress(void *dest, size_t dest_size,
-const void *src, size_t src_size)
+static ssize_t qcow2_zlib_decompress(void *dest, size_t dest_size,
+ const void *src, size_t src_size)
 {
 int ret;
 z_stream strm;
@@ -191,20 +193,67 @@ qcow2_co_do_compress(BlockDriverState *bs, void *dest, 
size_t dest_size,
 return arg.ret;
 }
 
+/*
+ * qcow2_co_compress()
+ *
+ * Compress @src_size bytes of data using the compression
+ * method defined by the image compression type
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: compressed size on success
+ *  a negative error code on failure
+ */
 ssize_t coroutine_fn
 qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
   const void *src, size_t src_size)
 {
-return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
-qcow2_compress);
+BDRVQcow2State *s = bs->opaque;
+Qcow2CompressFunc fn;
+
+switch (s->compression_type) {
+case QCOW2_COMPRESSION_TYPE_ZLIB:
+fn = qcow2_zlib_compress;
+break;
+
+default:
+abort();
+}
+
+return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
 }
 
+/*
+ * qcow2_co_decompress()
+ *
+ * Decompress some data (not more than @src_size bytes) to produce exactly
+ * @dest_size bytes using the compression method defined by the image
+ * compression type
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: 0 on success
+ *  a negative error code on failure
+ */
 ssize_t coroutine_fn
 qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
 const void *src, size_t src_size)
 {
-return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
-qcow2_decompress);
+BDRVQcow2State *s = bs->opaque;
+Qcow2CompressFunc fn;
+
+switch (s->compression_type) {
+case QCOW2_COMPRESSION_TYPE_ZLIB:
+fn = qcow2_zlib_decompress;
+break;
+
+default:
+abort();
+}
+
+return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
 }
 
 
-- 
2.17.0

[PATCH v11 0/4] qcow2: Implement zstd cluster compression method

[Denis Plotnikov]

v11:
   * 03: the loops don't need "do{}while" form anymore and
 the they were buggy (missed "do" in the beginning)
 replace them with usual "while(){}" loops [Vladimir]
v10:
   * 03: fix zstd (de)compressed loops for multi-frame
 cases [Vladimir]
v9:
   * 01: fix error checking and reporting in qcow2_amend compression type part 
[Vladimir]
   * 03: replace asserts with -EIO in qcow2_zstd_decompression [Vladimir, 
Alberto]
   * 03: reword/amend/add comments, fix typos [Vladimir]

v8:
   * 03: switch zstd API from simple to stream [Eric]
 No need to state a special cluster layout for zstd
 compressed clusters.
v7:
   * use qapi_enum_parse instead of the open-coding [Eric]
   * fix wording, typos and spelling [Eric]

v6:
   * "block/qcow2-threads: fix qcow2_decompress" is removed from the series
  since it has been accepted by Max already
   * add compile time checking for Qcow2Header to be a multiple of 8 [Max, 
Alberto]
   * report error on qcow2 amending when the compression type is actually 
chnged [Max]
   * remove the extra space and the extra new line [Max]
   * re-arrange acks and signed-off-s [Vladimir]

v5:
   * replace -ENOTSUP with abort in qcow2_co_decompress [Vladimir]
   * set cluster size for all test cases in the beginning of the 287 test

v4:
   * the series is rebased on top of 01 "block/qcow2-threads: fix 
qcow2_decompress"
   * 01 is just a no-change resend to avoid extra dependencies. Still, it may 
be merged in separate

v3:
   * remove redundant max compression type value check [Vladimir, Eric]
 (the switch below checks everything)
   * prevent compression type changing on "qemu-img amend" [Vladimir]
   * remove zstd config setting, since it has been added already by
 "migration" patches [Vladimir]
   * change the compression type error message [Vladimir] 
   * fix alignment and 80-chars exceeding [Vladimir]

v2:
   * rework compression type setting [Vladimir]
   * squash iotest changes to the compression type introduction patch 
[Vladimir, Eric]
   * fix zstd availability checking in zstd iotest [Vladimir]
   * remove unnecessry casting [Eric]
   * remove rudundant checks [Eric]
   * fix compressed cluster layout in qcow2 spec [Vladimir]
   * fix wording [Eric, Vladimir]
   * fix compression type filtering in iotests [Eric]

v1:
   the initial series

Denis Plotnikov (4):
  qcow2: introduce compression type feature
  qcow2: rework the cluster compression routine
  qcow2: add zstd cluster compression
  iotests: 287: add qcow2 compression type test

 docs/interop/qcow2.txt   |   1 +
 configure|   2 +-
 qapi/block-core.json |  23 +++-
 block/qcow2.h|  20 ++-
 include/block/block_int.h|   1 +
 block/qcow2-threads.c| 209 +--
 block/qcow2.c| 120 ++
 tests/qemu-iotests/031.out   |  14 +--
 tests/qemu-iotests/036.out   |   4 +-
 tests/qemu-iotests/049.out   | 102 +++
 tests/qemu-iotests/060.out   |   1 +
 tests/qemu-iotests/061.out   |  34 ++---
 tests/qemu-iotests/065   |  28 +++--
 tests/qemu-iotests/080   |   2 +-
 tests/qemu-iotests/144.out   |   4 +-
 tests/qemu-iotests/182.out   |   2 +-
 tests/qemu-iotests/242.out   |   5 +
 tests/qemu-iotests/255.out   |   8 +-
 tests/qemu-iotests/287   | 128 +++
 tests/qemu-iotests/287.out   |  43 +++
 tests/qemu-iotests/common.filter |   3 +-
 tests/qemu-iotests/group |   1 +
 22 files changed, 647 insertions(+), 108 deletions(-)
 create mode 100755 tests/qemu-iotests/287
 create mode 100644 tests/qemu-iotests/287.out

-- 
2.17.0

[PATCH v11 1/4] qcow2: introduce compression type feature

[Denis Plotnikov]

The patch adds some preparation parts for incompatible compression type
feature to qcow2 allowing the use different compression methods for
image clusters (de)compressing.

It is implied that the compression type is set on the image creation and
can be changed only later by image conversion, thus compression type
defines the only compression algorithm used for the image, and thus,
for all image clusters.

The goal of the feature is to add support of other compression methods
to qcow2. For example, ZSTD which is more effective on compression than ZLIB.

The default compression is ZLIB. Images created with ZLIB compression type
are backward compatible with older qemu versions.

Adding of the compression type breaks a number of tests because now the
compression type is reported on image creation and there are some changes
in the qcow2 header in size and offsets.

The tests are fixed in the following ways:
* filter out compression_type for many tests
* fix header size, feature table size and backing file offset
  affected tests: 031, 036, 061, 080
  header_size +=8: 1 byte compression type
   7 bytes padding
  feature_table += 48: incompatible feature compression type
  backing_file_offset += 56 (8 + 48 -> header_change + feature_table_change)
* add "compression type" for test output matching when it isn't filtered
  affected tests: 049, 060, 061, 065, 144, 182, 242, 255

Signed-off-by: Denis Plotnikov 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 qapi/block-core.json |  22 +-
 block/qcow2.h|  20 +-
 include/block/block_int.h|   1 +
 block/qcow2.c| 113 +++
 tests/qemu-iotests/031.out   |  14 ++--
 tests/qemu-iotests/036.out   |   4 +-
 tests/qemu-iotests/049.out   | 102 ++--
 tests/qemu-iotests/060.out   |   1 +
 tests/qemu-iotests/061.out   |  34 ++
 tests/qemu-iotests/065   |  28 +---
 tests/qemu-iotests/080   |   2 +-
 tests/qemu-iotests/144.out   |   4 +-
 tests/qemu-iotests/182.out   |   2 +-
 tests/qemu-iotests/242.out   |   5 ++
 tests/qemu-iotests/255.out   |   8 +--
 tests/qemu-iotests/common.filter |   3 +-
 16 files changed, 267 insertions(+), 96 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 943df1926a..d669ec0965 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -78,6 +78,8 @@
 #
 # @bitmaps: A list of qcow2 bitmap details (since 4.0)
 #
+# @compression-type: the image cluster compression method (since 5.0)
+#
 # Since: 1.7
 ##
 { 'struct': 'ImageInfoSpecificQCow2',
@@ -89,7 +91,8 @@
   '*corrupt': 'bool',
   'refcount-bits': 'int',
   '*encrypt': 'ImageInfoSpecificQCow2Encryption',
-  '*bitmaps': ['Qcow2BitmapInfo']
+  '*bitmaps': ['Qcow2BitmapInfo'],
+  'compression-type': 'Qcow2CompressionType'
   } }
 
 ##
@@ -4284,6 +4287,18 @@
   'data': [ 'v2', 'v3' ] }
 
 
+##
+# @Qcow2CompressionType:
+#
+# Compression type used in qcow2 image file
+#
+# @zlib: zlib compression, see 
+#
+# Since: 5.0
+##
+{ 'enum': 'Qcow2CompressionType',
+  'data': [ 'zlib' ] }
+
 ##
 # @BlockdevCreateOptionsQcow2:
 #
@@ -4307,6 +4322,8 @@
 # allowed values: off, falloc, full, metadata)
 # @lazy-refcounts: True if refcounts may be updated lazily (default: off)
 # @refcount-bits: Width of reference counts in bits (default: 16)
+# @compression-type: The image cluster compression method
+#(default: zlib, since 5.0)
 #
 # Since: 2.12
 ##
@@ -4322,7 +4339,8 @@
 '*cluster-size':'size',
 '*preallocation':   'PreallocMode',
 '*lazy-refcounts':  'bool',
-'*refcount-bits':   'int' } }
+'*refcount-bits':   'int',
+'*compression-type':'Qcow2CompressionType' } }
 
 ##
 # @BlockdevCreateOptionsQed:
diff --git a/block/qcow2.h b/block/qcow2.h
index 0942126232..fca2fe33f9 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -146,8 +146,16 @@ typedef struct QCowHeader {
 
 uint32_t refcount_order;
 uint32_t header_length;
+
+/* Additional fields */
+uint8_t compression_type;
+
+/* header must be a multiple of 8 */
+uint8_t padding[7];
 } QEMU_PACKED QCowHeader;
 
+QEMU_BUILD_BUG_ON(!QEMU_IS_ALIGNED(sizeof(QCowHeader), 8));
+
 typedef struct QEMU_PACKED QCowSnapshotHeader {
 /* header is 8 byte aligned */
 uint64_t l1_table_offset;
@@ -216,13 +224,16 @@ enum {
 QCOW2_INCOMPAT_DIRTY_BITNR  = 0,
 QCOW2_INCOMPAT_CORRUPT_BITNR= 1,
 QCOW2_INCOMPAT_DATA_FILE_BITNR  = 2,
+QCOW2_INCOMPAT_COMPRESSION_BITNR = 3,
 QCOW2_INCOMPAT_DIRTY= 1 << QCOW2_INCOMPAT_DIRTY_BITNR,
 QCOW2_INCOMPAT_CORRUPT  = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR,
 QCOW2_INCOMPAT_DATA_FILE= 1 << QCOW2_INCOMPAT_DATA_FILE_BITNR,
+QCOW2_INCOMPAT_COMPRESSION  = 

[PATCH v11 3/4] qcow2: add zstd cluster compression

[Denis Plotnikov]

zstd significantly reduces cluster compression time.
It provides better compression performance maintaining
the same level of the compression ratio in comparison with
zlib, which, at the moment, is the only compression
method available.

The performance test results:
Test compresses and decompresses qemu qcow2 image with just
installed rhel-7.6 guest.
Image cluster size: 64K. Image on disk size: 2.2G

The test was conducted with brd disk to reduce the influence
of disk subsystem to the test results.
The results is given in seconds.

compress cmd:
  time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
  src.img [zlib|zstd]_compressed.img
decompress cmd
  time ./qemu-img convert -O qcow2
  [zlib|zstd]_compressed.img uncompressed.img

   compression   decompression
 zlib   zstd   zlib zstd

real 65.5   16.3 (-75 %)1.9  1.6 (-16 %)
user 65.0   15.85.3  2.5
sys   3.30.22.0  2.0

Both ZLIB and ZSTD gave the same compression ratio: 1.57
compressed image size in both cases: 1.4G

Signed-off-by: Denis Plotnikov 
QAPI part:
Acked-by: Markus Armbruster 
---
 docs/interop/qcow2.txt |   1 +
 configure  |   2 +-
 qapi/block-core.json   |   3 +-
 block/qcow2-threads.c  | 138 +
 block/qcow2.c  |   7 +++
 5 files changed, 149 insertions(+), 2 deletions(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 5597e24474..795dbb21dd 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -208,6 +208,7 @@ version 2.
 
 Available compression type values:
 0: zlib 
+1: zstd 
 
 
 === Header padding ===
diff --git a/configure b/configure
index da09c35895..57cac2abe1 100755
--- a/configure
+++ b/configure
@@ -1861,7 +1861,7 @@ disabled with --disable-FEATURE, default is enabled if 
available:
   lzfse   support of lzfse compression library
   (for reading lzfse-compressed dmg images)
   zstdsupport for zstd compression library
-  (for migration compression)
+  (for migration compression and qcow2 cluster compression)
   seccomp seccomp support
   coroutine-pool  coroutine freelist (better performance)
   glusterfs   GlusterFS backend
diff --git a/qapi/block-core.json b/qapi/block-core.json
index d669ec0965..44690ed266 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4293,11 +4293,12 @@
 # Compression type used in qcow2 image file
 #
 # @zlib: zlib compression, see 
+# @zstd: zstd compression, see 
 #
 # Since: 5.0
 ##
 { 'enum': 'Qcow2CompressionType',
-  'data': [ 'zlib' ] }
+  'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
 
 ##
 # @BlockdevCreateOptionsQcow2:
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 7dbaf53489..b8ccd97009 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -28,6 +28,11 @@
 #define ZLIB_CONST
 #include 
 
+#ifdef CONFIG_ZSTD
+#include 
+#include 
+#endif
+
 #include "qcow2.h"
 #include "block/thread-pool.h"
 #include "crypto.h"
@@ -166,6 +171,129 @@ static ssize_t qcow2_zlib_decompress(void *dest, size_t 
dest_size,
 return ret;
 }
 
+#ifdef CONFIG_ZSTD
+
+/*
+ * qcow2_zstd_compress()
+ *
+ * Compress @src_size bytes of data using zstd compression method
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: compressed size on success
+ *  -ENOMEM destination buffer is not enough to store compressed data
+ *  -EIOon any other error
+ */
+static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
+   const void *src, size_t src_size)
+{
+size_t ret;
+ZSTD_outBuffer output = { dest, dest_size, 0 };
+ZSTD_inBuffer input = { src, src_size, 0 };
+ZSTD_CCtx *cctx = ZSTD_createCCtx();
+
+if (!cctx) {
+return -EIO;
+}
+/*
+ * ZSTD spec: "You must continue calling ZSTD_compressStream2()
+ * with ZSTD_e_end until it returns 0, at which point you are
+ * free to start a new frame".
+ * In the loop, we try to compress all the data into one zstd frame.
+ * ZSTD_compressStream2 can potentially finish a frame earlier
+ * than the full input data is consumed. That's why we are looping
+ * until all the input data is consumed.
+ */
+while (input.pos < input.size) {
+/*
+ * zstd simple interface requires the exact compressed size.
+ * zstd stream interface reads the comressed size from
+ * the compressed stream frame.
+ * Instruct zstd to compress the whole 

[PATCH v11 4/4] iotests: 287: add qcow2 compression type test

[Denis Plotnikov]

The test checks fulfilling qcow2 requiriements for the compression
type feature and zstd compression type operability.

Signed-off-by: Denis Plotnikov 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/287 | 128 +
 tests/qemu-iotests/287.out |  43 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 172 insertions(+)
 create mode 100755 tests/qemu-iotests/287
 create mode 100644 tests/qemu-iotests/287.out

diff --git a/tests/qemu-iotests/287 b/tests/qemu-iotests/287
new file mode 100755
index 00..49d15b3d43
--- /dev/null
+++ b/tests/qemu-iotests/287
@@ -0,0 +1,128 @@
+#!/usr/bin/env bash
+#
+# Test case for an image using zstd compression
+#
+# Copyright (c) 2020 Virtuozzo International GmbH
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=dplotni...@virtuozzo.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+status=1   # failure is the default!
+
+_cleanup()
+{
+   _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# standard environment
+. ./common.rc
+. ./common.filter
+
+# This tests qocw2-specific low-level functionality
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+# for all the cases
+CLUSTER_SIZE=65536
+
+# Check if we can run this test.
+
+IMGOPTS='compression_type=zstd' _make_test_img 64M | grep "Invalid parameter 
'zstd'" 2>&1 1>/dev/null
+
+ZSTD_SUPPORTED=$?
+
+if (($ZSTD_SUPPORTED==0)); then
+_notrun "ZSTD is disabled"
+fi
+
+# Test: when compression is zlib the incompatible bit is unset
+echo
+echo "=== Testing compression type incompatible bit setting for zlib ==="
+echo
+
+IMGOPTS='compression_type=zlib' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+
+# Test: when compression differs from zlib the incompatible bit is set
+echo
+echo "=== Testing compression type incompatible bit setting for zstd ==="
+echo
+
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+
+# Test: an image can't be openned if compression type is zlib and
+#   incompatible feature compression type is set
+echo
+echo "=== Testing zlib with incompatible bit set  ==="
+echo
+
+IMGOPTS='compression_type=zlib' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" set-feature-bit incompatible 3
+# to make sure the bit was actually set
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+$QEMU_IMG info "$TEST_IMG" 2>1 1>/dev/null
+if (($?==0)); then
+echo "Error: The image openned successfully. The image must not be openned"
+fi
+
+# Test: an image can't be openned if compression type is NOT zlib and
+#   incompatible feature compression type is UNSET
+echo
+echo "=== Testing zstd with incompatible bit unset  ==="
+echo
+
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" set-header incompatible_features 0
+# to make sure the bit was actually unset
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+$QEMU_IMG info "$TEST_IMG" 2>1 1>/dev/null
+if (($?==0)); then
+echo "Error: The image openned successfully. The image must not be openned"
+fi
+# Test: check compression type values
+echo
+echo "=== Testing compression type values  ==="
+echo
+# zlib=0
+IMGOPTS='compression_type=zlib' _make_test_img 64M
+od -j104 -N1 -An -vtu1 "$TEST_IMG"
+
+# zstd=1
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+od -j104 -N1 -An -vtu1 "$TEST_IMG"
+
+# Test: using zstd compression, write to and read from an image
+echo
+echo "=== Testing reading and writing with zstd ==="
+echo
+
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+$QEMU_IO -c "write -c -P 0xAC 65536 64k " "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -P 0xAC 65536 65536 " "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -v 131070 8 " "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -v 65534 8" "$TEST_IMG" | _filter_qemu_io
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/287.out b/tests/qemu-iotests/287.out
new file mode 100644
index 00..8e51c3078d
--- /dev/null
+++ b/tests/qemu-iotests/287.out
@@ -0,0 +1,43 @@
+QA output created by 287
+
+=== Testing compression type incompatible bit setting for zlib ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864

Re: AIO_WAIT_WHILE questions

[Stefan Hajnoczi]

On Mon, Mar 30, 2020 at 10:09:45AM +0200, Markus Armbruster wrote:
> Cc'ing people based on output of "scripts/get_maintainer.pl -f
> include/block/aio-wait.h".
> 
> Dietmar Maurer  writes:
> 
> > Hi all,
> >
> > I have a question about AIO_WAIT_WHILE. The docs inside the code say:
> >
> >  * The caller's thread must be the IOThread that owns @ctx or the main loop
> >  * thread (with @ctx acquired exactly once).
> >
> > I wonder if that "with @ctx acquired exactly once" is always required?
> >
> > I have done a quick test (see code below) and this reveals that the 
> > condition is not
> > always met.
> >
> > Or is my test wrong (or the docs)?
> >
> > ---debug helper---
> > diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h
> > index afeeb18f95..cf78dca9f9 100644
> > --- a/include/block/aio-wait.h
> > +++ b/include/block/aio-wait.h
> > @@ -82,6 +82,8 @@ extern AioWait global_aio_wait;
> >  atomic_inc(_->num_waiters);   \
> >  if (ctx_ && in_aio_context_home_thread(ctx_)) {\
> >  while ((cond)) {   \
> > +printf("AIO_WAIT_WHILE %p %d\n", ctx, ctx_->lock_count); \
> > +assert(ctx_->lock_count == 1);   \
> >  aio_poll(ctx_, true);  \
> >  waited_ = true;\
> >  }  \

In this case it doesn't matter.  Handlers invoked by aio_poll() that
acquire ctx's recursive mutex will succeed.

The "exactly once" requirement is there because nested locking is not
supported when waiting for an AioContext that runs in a different
thread:

} else {   \
assert(qemu_get_current_aio_context() ==   \
   qemu_get_aio_context());\
while ((cond)) {   \
if (ctx_) {\
aio_context_release(ctx_); \
^--- doesn't work if we have acquired it multiple times

I think it would be okay to update the documentation to make this clear.


signature.asc
Description: PGP signature

Re: [PATCH v4 1/2] virtio-blk: delete vqs on the error path in realize()

[Stefan Hajnoczi]

On Sat, Mar 28, 2020 at 08:57:04AM +0800, Pan Nengyuan wrote:
> virtio_vqs forgot to free on the error path in realize(). Fix that.
> 
> The asan stack:
> Direct leak of 14336 byte(s) in 1 object(s) allocated from:
> #0 0x7f58b93fd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
> #1 0x7f58b858249d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
> #2 0x5562cc627f49 in virtio_add_queue 
> /mnt/sdb/qemu/hw/virtio/virtio.c:2413
> #3 0x5562cc4b524a in virtio_blk_device_realize 
> /mnt/sdb/qemu/hw/block/virtio-blk.c:1202
> #4 0x5562cc613050 in virtio_device_realize 
> /mnt/sdb/qemu/hw/virtio/virtio.c:3615
> #5 0x5562ccb7a568 in device_set_realized /mnt/sdb/qemu/hw/core/qdev.c:891
> #6 0x5562cd39cd45 in property_set_bool /mnt/sdb/qemu/qom/object.c:2238
> 
> Reported-by: Euler Robot 
> Signed-off-by: Pan Nengyuan 
> Reviewed-by: Stefano Garzarella 
> ---
> v2->v1:
> - Fix incorrect free in v1, it will cause a uaf.
> ---
> Cc: Stefan Hajnoczi 
> Cc: Kevin Wolf 
> Cc: Max Reitz 
> Cc: qemu-block@nongnu.org
> ---
>  hw/block/virtio-blk.c | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: AIO_WAIT_WHILE questions

[Markus Armbruster]

Cc'ing people based on output of "scripts/get_maintainer.pl -f
include/block/aio-wait.h".

Dietmar Maurer  writes:

> Hi all,
>
> I have a question about AIO_WAIT_WHILE. The docs inside the code say:
>
>  * The caller's thread must be the IOThread that owns @ctx or the main loop
>  * thread (with @ctx acquired exactly once).
>
> I wonder if that "with @ctx acquired exactly once" is always required?
>
> I have done a quick test (see code below) and this reveals that the condition 
> is not
> always met.
>
> Or is my test wrong (or the docs)?
>
> ---debug helper---
> diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h
> index afeeb18f95..cf78dca9f9 100644
> --- a/include/block/aio-wait.h
> +++ b/include/block/aio-wait.h
> @@ -82,6 +82,8 @@ extern AioWait global_aio_wait;
>  atomic_inc(_->num_waiters);   \
>  if (ctx_ && in_aio_context_home_thread(ctx_)) {\
>  while ((cond)) {   \
> +printf("AIO_WAIT_WHILE %p %d\n", ctx, ctx_->lock_count); \
> +assert(ctx_->lock_count == 1);   \
>  aio_poll(ctx_, true);  \
>  waited_ = true;\
>  }  \
> diff --git a/include/block/aio.h b/include/block/aio.h
> index cb1989105a..51ef20e2f0 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -125,6 +125,7 @@ struct AioContext {
>  
>  /* Used by AioContext users to protect from multi-threaded access.  */
>  QemuRecMutex lock;
> +int lock_count;
>  
>  /* The list of registered AIO handlers.  Protected by ctx->list_lock. */
>  AioHandlerList aio_handlers;
> diff --git a/util/async.c b/util/async.c
> index b94518b948..9804c6c64f 100644
> --- a/util/async.c
> +++ b/util/async.c
> @@ -594,9 +594,11 @@ void aio_context_unref(AioContext *ctx)
>  void aio_context_acquire(AioContext *ctx)
>  {
>  qemu_rec_mutex_lock(>lock);
> +ctx->lock_count++;
>  }
>  
>  void aio_context_release(AioContext *ctx)
>  {
> +ctx->lock_count--;
>  qemu_rec_mutex_unlock(>lock);
>  }

[PATCH v10 3/4] qcow2: add zstd cluster compression

[Denis Plotnikov]

zstd significantly reduces cluster compression time.
It provides better compression performance maintaining
the same level of the compression ratio in comparison with
zlib, which, at the moment, is the only compression
method available.

The performance test results:
Test compresses and decompresses qemu qcow2 image with just
installed rhel-7.6 guest.
Image cluster size: 64K. Image on disk size: 2.2G

The test was conducted with brd disk to reduce the influence
of disk subsystem to the test results.
The results is given in seconds.

compress cmd:
  time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
  src.img [zlib|zstd]_compressed.img
decompress cmd
  time ./qemu-img convert -O qcow2
  [zlib|zstd]_compressed.img uncompressed.img

   compression   decompression
 zlib   zstd   zlib zstd

real 65.5   16.3 (-75 %)1.9  1.6 (-16 %)
user 65.0   15.85.3  2.5
sys   3.30.22.0  2.0

Both ZLIB and ZSTD gave the same compression ratio: 1.57
compressed image size in both cases: 1.4G

Signed-off-by: Denis Plotnikov 
QAPI part:
Acked-by: Markus Armbruster 
---
 docs/interop/qcow2.txt |   1 +
 configure  |   2 +-
 qapi/block-core.json   |   3 +-
 block/qcow2-threads.c  | 140 +
 block/qcow2.c  |   7 +++
 5 files changed, 151 insertions(+), 2 deletions(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 5597e24474..795dbb21dd 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -208,6 +208,7 @@ version 2.
 
 Available compression type values:
 0: zlib 
+1: zstd 
 
 
 === Header padding ===
diff --git a/configure b/configure
index caa65f5883..b2a0aa241a 100755
--- a/configure
+++ b/configure
@@ -1835,7 +1835,7 @@ disabled with --disable-FEATURE, default is enabled if 
available:
   lzfse   support of lzfse compression library
   (for reading lzfse-compressed dmg images)
   zstdsupport for zstd compression library
-  (for migration compression)
+  (for migration compression and qcow2 cluster compression)
   seccomp seccomp support
   coroutine-pool  coroutine freelist (better performance)
   glusterfs   GlusterFS backend
diff --git a/qapi/block-core.json b/qapi/block-core.json
index a306484973..8953451818 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4401,11 +4401,12 @@
 # Compression type used in qcow2 image file
 #
 # @zlib: zlib compression, see 
+# @zstd: zstd compression, see 
 #
 # Since: 5.0
 ##
 { 'enum': 'Qcow2CompressionType',
-  'data': [ 'zlib' ] }
+  'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
 
 ##
 # @BlockdevCreateOptionsQcow2:
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 7dbaf53489..410953ce76 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -28,6 +28,11 @@
 #define ZLIB_CONST
 #include 
 
+#ifdef CONFIG_ZSTD
+#include 
+#include 
+#endif
+
 #include "qcow2.h"
 #include "block/thread-pool.h"
 #include "crypto.h"
@@ -166,6 +171,131 @@ static ssize_t qcow2_zlib_decompress(void *dest, size_t 
dest_size,
 return ret;
 }
 
+#ifdef CONFIG_ZSTD
+
+/*
+ * qcow2_zstd_compress()
+ *
+ * Compress @src_size bytes of data using zstd compression method
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: compressed size on success
+ *  -ENOMEM destination buffer is not enough to store compressed data
+ *  -EIOon any other error
+ */
+static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
+   const void *src, size_t src_size)
+{
+size_t ret;
+ZSTD_outBuffer output = { dest, dest_size, 0 };
+ZSTD_inBuffer input = { src, src_size, 0 };
+ZSTD_CCtx *cctx = ZSTD_createCCtx();
+
+if (!cctx) {
+return -EIO;
+}
+/*
+ * ZSTD spec: "You must continue calling ZSTD_compressStream2()
+ * with ZSTD_e_end until it returns 0, at which point you are
+ * free to start a new frame".
+ * In the loop, we try to compress all the data into one zstd frame.
+ * ZSTD_compressStream2 can potentially finish a frame earlier
+ * than the full input data is consumed. That's why we are looping
+ * until all the input data is consumed.
+ */
+{
+/*
+ * zstd simple interface requires the exact compressed size.
+ * zstd stream interface reads the comressed size from
+ * the compressed stream frame.
+ * Instruct zstd to compress the whole buffer and write
+ * the 

[PATCH v10 2/4] qcow2: rework the cluster compression routine

[Denis Plotnikov]

The patch enables processing the image compression type defined
for the image and chooses an appropriate method for image clusters
(de)compression.

Signed-off-by: Denis Plotnikov 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Alberto Garcia 
---
 block/qcow2-threads.c | 71 ---
 1 file changed, 60 insertions(+), 11 deletions(-)

diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index a68126f291..7dbaf53489 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -74,7 +74,9 @@ typedef struct Qcow2CompressData {
 } Qcow2CompressData;
 
 /*
- * qcow2_compress()
+ * qcow2_zlib_compress()
+ *
+ * Compress @src_size bytes of data using zlib compression method
  *
  * @dest - destination buffer, @dest_size bytes
  * @src - source buffer, @src_size bytes
@@ -83,8 +85,8 @@ typedef struct Qcow2CompressData {
  *  -ENOMEM destination buffer is not enough to store compressed data
  *  -EIOon any other error
  */
-static ssize_t qcow2_compress(void *dest, size_t dest_size,
-  const void *src, size_t src_size)
+static ssize_t qcow2_zlib_compress(void *dest, size_t dest_size,
+   const void *src, size_t src_size)
 {
 ssize_t ret;
 z_stream strm;
@@ -119,10 +121,10 @@ static ssize_t qcow2_compress(void *dest, size_t 
dest_size,
 }
 
 /*
- * qcow2_decompress()
+ * qcow2_zlib_decompress()
  *
  * Decompress some data (not more than @src_size bytes) to produce exactly
- * @dest_size bytes.
+ * @dest_size bytes using zlib compression method
  *
  * @dest - destination buffer, @dest_size bytes
  * @src - source buffer, @src_size bytes
@@ -130,8 +132,8 @@ static ssize_t qcow2_compress(void *dest, size_t dest_size,
  * Returns: 0 on success
  *  -EIO on fail
  */
-static ssize_t qcow2_decompress(void *dest, size_t dest_size,
-const void *src, size_t src_size)
+static ssize_t qcow2_zlib_decompress(void *dest, size_t dest_size,
+ const void *src, size_t src_size)
 {
 int ret;
 z_stream strm;
@@ -191,20 +193,67 @@ qcow2_co_do_compress(BlockDriverState *bs, void *dest, 
size_t dest_size,
 return arg.ret;
 }
 
+/*
+ * qcow2_co_compress()
+ *
+ * Compress @src_size bytes of data using the compression
+ * method defined by the image compression type
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: compressed size on success
+ *  a negative error code on failure
+ */
 ssize_t coroutine_fn
 qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
   const void *src, size_t src_size)
 {
-return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
-qcow2_compress);
+BDRVQcow2State *s = bs->opaque;
+Qcow2CompressFunc fn;
+
+switch (s->compression_type) {
+case QCOW2_COMPRESSION_TYPE_ZLIB:
+fn = qcow2_zlib_compress;
+break;
+
+default:
+abort();
+}
+
+return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
 }
 
+/*
+ * qcow2_co_decompress()
+ *
+ * Decompress some data (not more than @src_size bytes) to produce exactly
+ * @dest_size bytes using the compression method defined by the image
+ * compression type
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: 0 on success
+ *  a negative error code on failure
+ */
 ssize_t coroutine_fn
 qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
 const void *src, size_t src_size)
 {
-return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
-qcow2_decompress);
+BDRVQcow2State *s = bs->opaque;
+Qcow2CompressFunc fn;
+
+switch (s->compression_type) {
+case QCOW2_COMPRESSION_TYPE_ZLIB:
+fn = qcow2_zlib_decompress;
+break;
+
+default:
+abort();
+}
+
+return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
 }
 
 
-- 
2.17.0

[PATCH v10 1/4] qcow2: introduce compression type feature

[Denis Plotnikov]

The patch adds some preparation parts for incompatible compression type
feature to qcow2 allowing the use different compression methods for
image clusters (de)compressing.

It is implied that the compression type is set on the image creation and
can be changed only later by image conversion, thus compression type
defines the only compression algorithm used for the image, and thus,
for all image clusters.

The goal of the feature is to add support of other compression methods
to qcow2. For example, ZSTD which is more effective on compression than ZLIB.

The default compression is ZLIB. Images created with ZLIB compression type
are backward compatible with older qemu versions.

Adding of the compression type breaks a number of tests because now the
compression type is reported on image creation and there are some changes
in the qcow2 header in size and offsets.

The tests are fixed in the following ways:
* filter out compression_type for many tests
* fix header size, feature table size and backing file offset
  affected tests: 031, 036, 061, 080
  header_size +=8: 1 byte compression type
   7 bytes padding
  feature_table += 48: incompatible feature compression type
  backing_file_offset += 56 (8 + 48 -> header_change + feature_table_change)
* add "compression type" for test output matching when it isn't filtered
  affected tests: 049, 060, 061, 065, 144, 182, 242, 255

Signed-off-by: Denis Plotnikov 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 qapi/block-core.json |  22 +-
 block/qcow2.h|  20 +-
 include/block/block_int.h|   1 +
 block/qcow2.c| 113 +++
 tests/qemu-iotests/031.out   |  14 ++--
 tests/qemu-iotests/036.out   |   4 +-
 tests/qemu-iotests/049.out   | 102 ++--
 tests/qemu-iotests/060.out   |   1 +
 tests/qemu-iotests/061.out   |  34 ++
 tests/qemu-iotests/065   |  28 +---
 tests/qemu-iotests/080   |   2 +-
 tests/qemu-iotests/144.out   |   4 +-
 tests/qemu-iotests/182.out   |   2 +-
 tests/qemu-iotests/242.out   |   5 ++
 tests/qemu-iotests/255.out   |   8 +--
 tests/qemu-iotests/common.filter |   3 +-
 16 files changed, 267 insertions(+), 96 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 85e27bb61f..a306484973 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -78,6 +78,8 @@
 #
 # @bitmaps: A list of qcow2 bitmap details (since 4.0)
 #
+# @compression-type: the image cluster compression method (since 5.0)
+#
 # Since: 1.7
 ##
 { 'struct': 'ImageInfoSpecificQCow2',
@@ -89,7 +91,8 @@
   '*corrupt': 'bool',
   'refcount-bits': 'int',
   '*encrypt': 'ImageInfoSpecificQCow2Encryption',
-  '*bitmaps': ['Qcow2BitmapInfo']
+  '*bitmaps': ['Qcow2BitmapInfo'],
+  'compression-type': 'Qcow2CompressionType'
   } }
 
 ##
@@ -4392,6 +4395,18 @@
   'data': [ 'v2', 'v3' ] }
 
 
+##
+# @Qcow2CompressionType:
+#
+# Compression type used in qcow2 image file
+#
+# @zlib: zlib compression, see 
+#
+# Since: 5.0
+##
+{ 'enum': 'Qcow2CompressionType',
+  'data': [ 'zlib' ] }
+
 ##
 # @BlockdevCreateOptionsQcow2:
 #
@@ -4415,6 +4430,8 @@
 # allowed values: off, falloc, full, metadata)
 # @lazy-refcounts: True if refcounts may be updated lazily (default: off)
 # @refcount-bits: Width of reference counts in bits (default: 16)
+# @compression-type: The image cluster compression method
+#(default: zlib, since 5.0)
 #
 # Since: 2.12
 ##
@@ -4430,7 +4447,8 @@
 '*cluster-size':'size',
 '*preallocation':   'PreallocMode',
 '*lazy-refcounts':  'bool',
-'*refcount-bits':   'int' } }
+'*refcount-bits':   'int',
+'*compression-type':'Qcow2CompressionType' } }
 
 ##
 # @BlockdevCreateOptionsQed:
diff --git a/block/qcow2.h b/block/qcow2.h
index 0942126232..fca2fe33f9 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -146,8 +146,16 @@ typedef struct QCowHeader {
 
 uint32_t refcount_order;
 uint32_t header_length;
+
+/* Additional fields */
+uint8_t compression_type;
+
+/* header must be a multiple of 8 */
+uint8_t padding[7];
 } QEMU_PACKED QCowHeader;
 
+QEMU_BUILD_BUG_ON(!QEMU_IS_ALIGNED(sizeof(QCowHeader), 8));
+
 typedef struct QEMU_PACKED QCowSnapshotHeader {
 /* header is 8 byte aligned */
 uint64_t l1_table_offset;
@@ -216,13 +224,16 @@ enum {
 QCOW2_INCOMPAT_DIRTY_BITNR  = 0,
 QCOW2_INCOMPAT_CORRUPT_BITNR= 1,
 QCOW2_INCOMPAT_DATA_FILE_BITNR  = 2,
+QCOW2_INCOMPAT_COMPRESSION_BITNR = 3,
 QCOW2_INCOMPAT_DIRTY= 1 << QCOW2_INCOMPAT_DIRTY_BITNR,
 QCOW2_INCOMPAT_CORRUPT  = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR,
 QCOW2_INCOMPAT_DATA_FILE= 1 << QCOW2_INCOMPAT_DATA_FILE_BITNR,
+QCOW2_INCOMPAT_COMPRESSION  = 

[PATCH v10 0/4] qcow2: Implement zstd cluster compression method

[Denis Plotnikov]

v10:
   * 03: fix zstd (de)compressed loops for multi-frame
 cases [Vladimir]
v9:
   * 01: fix error checking and reporting in qcow2_amend compression type part 
[Vladimir]
   * 03: replace asserts with -EIO in qcow2_zstd_decompression [Vladimir, 
Alberto]
   * 03: reword/amend/add comments, fix typos [Vladimir]

v8:
   * 03: switch zstd API from simple to stream [Eric]
 No need to state a special cluster layout for zstd
 compressed clusters.
v7:
   * use qapi_enum_parse instead of the open-coding [Eric]
   * fix wording, typos and spelling [Eric]

v6:
   * "block/qcow2-threads: fix qcow2_decompress" is removed from the series
  since it has been accepted by Max already
   * add compile time checking for Qcow2Header to be a multiple of 8 [Max, 
Alberto]
   * report error on qcow2 amending when the compression type is actually 
chnged [Max]
   * remove the extra space and the extra new line [Max]
   * re-arrange acks and signed-off-s [Vladimir]

v5:
   * replace -ENOTSUP with abort in qcow2_co_decompress [Vladimir]
   * set cluster size for all test cases in the beginning of the 287 test

v4:
   * the series is rebased on top of 01 "block/qcow2-threads: fix 
qcow2_decompress"
   * 01 is just a no-change resend to avoid extra dependencies. Still, it may 
be merged in separate

v3:
   * remove redundant max compression type value check [Vladimir, Eric]
 (the switch below checks everything)
   * prevent compression type changing on "qemu-img amend" [Vladimir]
   * remove zstd config setting, since it has been added already by
 "migration" patches [Vladimir]
   * change the compression type error message [Vladimir] 
   * fix alignment and 80-chars exceeding [Vladimir]

v2:
   * rework compression type setting [Vladimir]
   * squash iotest changes to the compression type introduction patch 
[Vladimir, Eric]
   * fix zstd availability checking in zstd iotest [Vladimir]
   * remove unnecessry casting [Eric]
   * remove rudundant checks [Eric]
   * fix compressed cluster layout in qcow2 spec [Vladimir]
   * fix wording [Eric, Vladimir]
   * fix compression type filtering in iotests [Eric]

v1:
   the initial series

Denis Plotnikov (4):
  qcow2: introduce compression type feature
  qcow2: rework the cluster compression routine
  qcow2: add zstd cluster compression
  iotests: 287: add qcow2 compression type test

 docs/interop/qcow2.txt   |   1 +
 configure|   2 +-
 qapi/block-core.json |  23 +++-
 block/qcow2.h|  20 ++-
 include/block/block_int.h|   1 +
 block/qcow2-threads.c| 211 +--
 block/qcow2.c| 120 ++
 tests/qemu-iotests/031.out   |  14 +-
 tests/qemu-iotests/036.out   |   4 +-
 tests/qemu-iotests/049.out   | 102 +++
 tests/qemu-iotests/060.out   |   1 +
 tests/qemu-iotests/061.out   |  34 +++--
 tests/qemu-iotests/065   |  28 ++--
 tests/qemu-iotests/080   |   2 +-
 tests/qemu-iotests/144.out   |   4 +-
 tests/qemu-iotests/182.out   |   2 +-
 tests/qemu-iotests/242.out   |   5 +
 tests/qemu-iotests/255.out   |   8 +-
 tests/qemu-iotests/287   | 128 +++
 tests/qemu-iotests/287.out   |  43 +++
 tests/qemu-iotests/common.filter |   3 +-
 tests/qemu-iotests/group |   1 +
 22 files changed, 649 insertions(+), 108 deletions(-)
 create mode 100755 tests/qemu-iotests/287
 create mode 100644 tests/qemu-iotests/287.out

-- 
2.17.0

[PATCH v10 4/4] iotests: 287: add qcow2 compression type test

[Denis Plotnikov]

The test checks fulfilling qcow2 requiriements for the compression
type feature and zstd compression type operability.

Signed-off-by: Denis Plotnikov 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/287 | 128 +
 tests/qemu-iotests/287.out |  43 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 172 insertions(+)
 create mode 100755 tests/qemu-iotests/287
 create mode 100644 tests/qemu-iotests/287.out

diff --git a/tests/qemu-iotests/287 b/tests/qemu-iotests/287
new file mode 100755
index 00..49d15b3d43
--- /dev/null
+++ b/tests/qemu-iotests/287
@@ -0,0 +1,128 @@
+#!/usr/bin/env bash
+#
+# Test case for an image using zstd compression
+#
+# Copyright (c) 2020 Virtuozzo International GmbH
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=dplotni...@virtuozzo.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+status=1   # failure is the default!
+
+_cleanup()
+{
+   _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# standard environment
+. ./common.rc
+. ./common.filter
+
+# This tests qocw2-specific low-level functionality
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+# for all the cases
+CLUSTER_SIZE=65536
+
+# Check if we can run this test.
+
+IMGOPTS='compression_type=zstd' _make_test_img 64M | grep "Invalid parameter 
'zstd'" 2>&1 1>/dev/null
+
+ZSTD_SUPPORTED=$?
+
+if (($ZSTD_SUPPORTED==0)); then
+_notrun "ZSTD is disabled"
+fi
+
+# Test: when compression is zlib the incompatible bit is unset
+echo
+echo "=== Testing compression type incompatible bit setting for zlib ==="
+echo
+
+IMGOPTS='compression_type=zlib' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+
+# Test: when compression differs from zlib the incompatible bit is set
+echo
+echo "=== Testing compression type incompatible bit setting for zstd ==="
+echo
+
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+
+# Test: an image can't be openned if compression type is zlib and
+#   incompatible feature compression type is set
+echo
+echo "=== Testing zlib with incompatible bit set  ==="
+echo
+
+IMGOPTS='compression_type=zlib' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" set-feature-bit incompatible 3
+# to make sure the bit was actually set
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+$QEMU_IMG info "$TEST_IMG" 2>1 1>/dev/null
+if (($?==0)); then
+echo "Error: The image openned successfully. The image must not be openned"
+fi
+
+# Test: an image can't be openned if compression type is NOT zlib and
+#   incompatible feature compression type is UNSET
+echo
+echo "=== Testing zstd with incompatible bit unset  ==="
+echo
+
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" set-header incompatible_features 0
+# to make sure the bit was actually unset
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+$QEMU_IMG info "$TEST_IMG" 2>1 1>/dev/null
+if (($?==0)); then
+echo "Error: The image openned successfully. The image must not be openned"
+fi
+# Test: check compression type values
+echo
+echo "=== Testing compression type values  ==="
+echo
+# zlib=0
+IMGOPTS='compression_type=zlib' _make_test_img 64M
+od -j104 -N1 -An -vtu1 "$TEST_IMG"
+
+# zstd=1
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+od -j104 -N1 -An -vtu1 "$TEST_IMG"
+
+# Test: using zstd compression, write to and read from an image
+echo
+echo "=== Testing reading and writing with zstd ==="
+echo
+
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+$QEMU_IO -c "write -c -P 0xAC 65536 64k " "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -P 0xAC 65536 65536 " "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -v 131070 8 " "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -v 65534 8" "$TEST_IMG" | _filter_qemu_io
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/287.out b/tests/qemu-iotests/287.out
new file mode 100644
index 00..8e51c3078d
--- /dev/null
+++ b/tests/qemu-iotests/287.out
@@ -0,0 +1,43 @@
+QA output created by 287
+
+=== Testing compression type incompatible bit setting for zlib ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864