Re: [PATCH 1/2] storage: change assigning qcow2 compat to 1.1 from 0.10 automatically

2024-03-05 Thread atp exp
On Tue, 5 Mar 2024 at 14:00, Peter Krempa  wrote:
>
> Just a note, please don't CC random people from the mailing list in the
> libvirt project.

In projects with faster patch frequencies, I typically CC maintainers
for efficient communication.
However, considering there's no separate MAINTAINERS file assigning
each module to a maintainer in this context. Thanks for the clarification.

> On Tue, Mar 05, 2024 at 01:50:32 +0530, Abhiram Tilak wrote:
> > In the file `storage/storage_util.c` currently `compat` varible is begin
> > assigned to 0.10 by default. This patch changes this default value to 1.1.
> >
> > This is done in efforts to upgrade the default qcow2 image version to
> > 1.1.
>
> We prefer if the commit message justifies the change rather than just
> summarizes it.
>
> The justification can be something along:
>
> storage: Use modern qcow2 by default
>
> Change the default to modern qcow2 as it's supported by all qemu
> versions supported by libvirt and in fact 'qemu-img' already defaults to
> the new format for a long time.
>
>
> >
> > Resolves: https://gitlab.com/libvirt/libvirt/-/issues/602
> > Signed-off-by: Abhiram Tilak 
> > ---
> >  src/storage/storage_util.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
> > index 7bf815d978..28d5fce4f0 100644
> > --- a/src/storage/storage_util.c
> > +++ b/src/storage/storage_util.c
> > @@ -765,7 +765,7 @@ 
> > storageBackendCreateQemuImgOpts(virStorageEncryptionInfoDef *encinfo,
> >  if (info->compat)
> >  virBufferAsprintf(, "compat=%s,", info->compat);
> >  else if (info->format == VIR_STORAGE_FILE_QCOW2)
> > -virBufferAddLit(, "compat=0.10,");
> > +virBufferAddLit(, "compat=1.1,");
> >
> >  if (info->clusterSize > 0)
> >  virBufferAsprintf(, "cluster_size=%llu,", info->clusterSize);
>
> Without all the changes in patch 2/2 this breaks the bulild. Our guides
> for sending patches state:
>
>   "If you're going to submit multiple patches, the automated tests must
>pass after each patch, not just after the last one."
>
> https://libvirt.org/hacking.html#preparing-patches
> You can add:
>
> Reviewed-by: Peter Krempa 
>
> for v2 with the new commit message and the test changes squashed into
> patch 1 so that tests pass.
>
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH 1/2] storage: change assigning qcow2 compat to 1.1 from 0.10 automatically

2024-03-05 Thread Peter Krempa
Just a note, please don't CC random people from the mailing list in the
libvirt project.

On Tue, Mar 05, 2024 at 01:50:32 +0530, Abhiram Tilak wrote:
> In the file `storage/storage_util.c` currently `compat` varible is begin
> assigned to 0.10 by default. This patch changes this default value to 1.1.
> 
> This is done in efforts to upgrade the default qcow2 image version to
> 1.1.

We prefer if the commit message justifies the change rather than just
summarizes it.

The justification can be something along:

storage: Use modern qcow2 by default

Change the default to modern qcow2 as it's supported by all qemu
versions supported by libvirt and in fact 'qemu-img' already defaults to
the new format for a long time.


> 
> Resolves: https://gitlab.com/libvirt/libvirt/-/issues/602
> Signed-off-by: Abhiram Tilak 
> ---
>  src/storage/storage_util.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
> index 7bf815d978..28d5fce4f0 100644
> --- a/src/storage/storage_util.c
> +++ b/src/storage/storage_util.c
> @@ -765,7 +765,7 @@ 
> storageBackendCreateQemuImgOpts(virStorageEncryptionInfoDef *encinfo,
>  if (info->compat)
>  virBufferAsprintf(, "compat=%s,", info->compat);
>  else if (info->format == VIR_STORAGE_FILE_QCOW2)
> -virBufferAddLit(, "compat=0.10,");
> +virBufferAddLit(, "compat=1.1,");
>  
>  if (info->clusterSize > 0)
>  virBufferAsprintf(, "cluster_size=%llu,", info->clusterSize);

Without all the changes in patch 2/2 this breaks the bulild. Our guides
for sending patches state:

  "If you're going to submit multiple patches, the automated tests must
   pass after each patch, not just after the last one."

https://libvirt.org/hacking.html#preparing-patches

You can add:

Reviewed-by: Peter Krempa 

for v2 with the new commit message and the test changes squashed into
patch 1 so that tests pass.
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


[PATCH 1/2] storage: change assigning qcow2 compat to 1.1 from 0.10 automatically

2024-03-04 Thread Abhiram Tilak
In the file `storage/storage_util.c` currently `compat` varible is begin
assigned to 0.10 by default. This patch changes this default value to 1.1.

This is done in efforts to upgrade the default qcow2 image version to
1.1.

Resolves: https://gitlab.com/libvirt/libvirt/-/issues/602
Signed-off-by: Abhiram Tilak 
---
 src/storage/storage_util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index 7bf815d978..28d5fce4f0 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -765,7 +765,7 @@ storageBackendCreateQemuImgOpts(virStorageEncryptionInfoDef 
*encinfo,
 if (info->compat)
 virBufferAsprintf(, "compat=%s,", info->compat);
 else if (info->format == VIR_STORAGE_FILE_QCOW2)
-virBufferAddLit(, "compat=0.10,");
+virBufferAddLit(, "compat=1.1,");
 
 if (info->clusterSize > 0)
 virBufferAsprintf(, "cluster_size=%llu,", info->clusterSize);
-- 
2.42.1
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org