1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH RFC] storage: Remove 'cow' disk format

Peter Krempa posted 1 patch 6 years, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/abb50d9e724b7d08c47cf3e37e3205bc7ca246dd.1521804659.git.pkrempa@redhat.com
Test syntax-check passed
docs/schemas/storagecommon.rng |  1 -
src/util/virstoragefile.c      | 31 +------------------------------
src/util/virstoragefile.h      |  3 +--
3 files changed, 2 insertions(+), 33 deletions(-)
[libvirt] [PATCH RFC] storage: Remove 'cow' disk format
Posted by Peter Krempa 6 years, 1 month ago 
There's no tests for this format and nothing seems to specifically care
about this format. QEMU does not even recognize it. Remove it completely.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 docs/schemas/storagecommon.rng |  1 -
 src/util/virstoragefile.c      | 31 +------------------------------
 src/util/virstoragefile.h      |  3 +--
 3 files changed, 2 insertions(+), 33 deletions(-)

diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng
index edee1b0845..310ed758ff 100644
--- a/docs/schemas/storagecommon.rng
+++ b/docs/schemas/storagecommon.rng
@@ -78,7 +78,6 @@
        we know how to follow backing chains, and all others -->
   <define name='storageFormatBacking'>
     <choice>
-      <value>cow</value>
       <value>qcow</value>
       <value>qcow2</value>
       <value>qed</value>
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 67b9ec71ac..dd07f09d2c 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -67,7 +67,7 @@ VIR_ENUM_IMPL(virStorageFileFormat,
               /* Not direct file formats, but used for various drivers */
               "fat", "vhd", "ploop",
               /* Formats with backing file below here */
-              "cow", "qcow", "qcow2", "qed", "vmdk")
+              "qcow", "qcow2", "qed", "vmdk")

 VIR_ENUM_IMPL(virStorageFileFeature,
               VIR_STORAGE_FILE_FEATURE_LAST,
@@ -170,8 +170,6 @@ struct FileTypeInfo {
 };


-static int cowGetBackingStore(char **, int *,
-                              const char *, size_t);
 static int qcow1GetBackingStore(char **, int *,
                                 const char *, size_t);
 static int qcow2GetBackingStore(char **, int *,
@@ -348,11 +346,6 @@ static struct FileTypeInfo const fileTypeInfo[] = {
                                  PLOOP_SIZE_MULTIPLIER, NULL, NULL, NULL },

     /* All formats with a backing store probe below here */
-    [VIR_STORAGE_FILE_COW] = {
-        0, "OOOM", NULL,
-        LV_BIG_ENDIAN, 4, 4, {2},
-        4+4+1024+4, 8, 1, NULL, cowGetBackingStore, NULL
-    },
     [VIR_STORAGE_FILE_QCOW] = {
         0, "QFI", NULL,
         LV_BIG_ENDIAN, 4, 4, {1},
@@ -397,28 +390,6 @@ static const int qcow2CompatibleFeatureArray[] = {
 verify(ARRAY_CARDINALITY(qcow2CompatibleFeatureArray) ==
        QCOW2_COMPATIBLE_FEATURE_LAST);

-static int
-cowGetBackingStore(char **res,
-                   int *format,
-                   const char *buf,
-                   size_t buf_size)
-{
-#define COW_FILENAME_MAXLEN 1024
-    *res = NULL;
-    *format = VIR_STORAGE_FILE_AUTO;
-
-    if (buf_size < 4+4+ COW_FILENAME_MAXLEN)
-        return BACKING_STORE_INVALID;
-    if (buf[4+4] == '\0') { /* cow_header_v2.backing_file[0] */
-        *format = VIR_STORAGE_FILE_NONE;
-        return BACKING_STORE_OK;
-    }
-
-    if (VIR_STRNDUP(*res, (const char*)buf + 4 + 4, COW_FILENAME_MAXLEN) < 0)
-        return BACKING_STORE_ERROR;
-    return BACKING_STORE_OK;
-}
-

 static int
 qcow2GetBackingStoreFormat(int *format,
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 596746ccb7..c84e013f46 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -82,8 +82,7 @@ typedef enum {
      * libvirt support for following a backing chain */
     VIR_STORAGE_FILE_BACKING,

-    VIR_STORAGE_FILE_COW = VIR_STORAGE_FILE_BACKING,
-    VIR_STORAGE_FILE_QCOW,
+    VIR_STORAGE_FILE_QCOW = VIR_STORAGE_FILE_BACKING,
     VIR_STORAGE_FILE_QCOW2,
     VIR_STORAGE_FILE_QED,
     VIR_STORAGE_FILE_VMDK,
-- 
2.16.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RFC] storage: Remove 'cow' disk format
Posted by Daniel P. Berrangé 6 years, 1 month ago 
On Fri, Mar 23, 2018 at 12:32:29PM +0100, Peter Krempa wrote:
> There's no tests for this format and nothing seems to specifically care
> about this format. QEMU does not even recognize it. Remove it completely.

This format is from the User Mode Linux driver, not QEMU.

> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  docs/schemas/storagecommon.rng |  1 -
>  src/util/virstoragefile.c      | 31 +------------------------------
>  src/util/virstoragefile.h      |  3 +--
>  3 files changed, 2 insertions(+), 33 deletions(-)
> 
> diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng
> index edee1b0845..310ed758ff 100644
> --- a/docs/schemas/storagecommon.rng
> +++ b/docs/schemas/storagecommon.rng
> @@ -78,7 +78,6 @@
>         we know how to follow backing chains, and all others -->
>    <define name='storageFormatBacking'>
>      <choice>
> -      <value>cow</value>
>        <value>qcow</value>
>        <value>qcow2</value>
>        <value>qed</value>
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 67b9ec71ac..dd07f09d2c 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -67,7 +67,7 @@ VIR_ENUM_IMPL(virStorageFileFormat,
>                /* Not direct file formats, but used for various drivers */
>                "fat", "vhd", "ploop",
>                /* Formats with backing file below here */
> -              "cow", "qcow", "qcow2", "qed", "vmdk")
> +              "qcow", "qcow2", "qed", "vmdk")
> 
>  VIR_ENUM_IMPL(virStorageFileFeature,
>                VIR_STORAGE_FILE_FEATURE_LAST,
> @@ -170,8 +170,6 @@ struct FileTypeInfo {
>  };
> 
> 
> -static int cowGetBackingStore(char **, int *,
> -                              const char *, size_t);
>  static int qcow1GetBackingStore(char **, int *,
>                                  const char *, size_t);
>  static int qcow2GetBackingStore(char **, int *,
> @@ -348,11 +346,6 @@ static struct FileTypeInfo const fileTypeInfo[] = {
>                                   PLOOP_SIZE_MULTIPLIER, NULL, NULL, NULL },
> 
>      /* All formats with a backing store probe below here */
> -    [VIR_STORAGE_FILE_COW] = {
> -        0, "OOOM", NULL,
> -        LV_BIG_ENDIAN, 4, 4, {2},
> -        4+4+1024+4, 8, 1, NULL, cowGetBackingStore, NULL
> -    },
>      [VIR_STORAGE_FILE_QCOW] = {
>          0, "QFI", NULL,
>          LV_BIG_ENDIAN, 4, 4, {1},
> @@ -397,28 +390,6 @@ static const int qcow2CompatibleFeatureArray[] = {
>  verify(ARRAY_CARDINALITY(qcow2CompatibleFeatureArray) ==
>         QCOW2_COMPATIBLE_FEATURE_LAST);
> 
> -static int
> -cowGetBackingStore(char **res,
> -                   int *format,
> -                   const char *buf,
> -                   size_t buf_size)
> -{
> -#define COW_FILENAME_MAXLEN 1024
> -    *res = NULL;
> -    *format = VIR_STORAGE_FILE_AUTO;
> -
> -    if (buf_size < 4+4+ COW_FILENAME_MAXLEN)
> -        return BACKING_STORE_INVALID;
> -    if (buf[4+4] == '\0') { /* cow_header_v2.backing_file[0] */
> -        *format = VIR_STORAGE_FILE_NONE;
> -        return BACKING_STORE_OK;
> -    }
> -
> -    if (VIR_STRNDUP(*res, (const char*)buf + 4 + 4, COW_FILENAME_MAXLEN) < 0)
> -        return BACKING_STORE_ERROR;
> -    return BACKING_STORE_OK;
> -}
> -
> 
>  static int
>  qcow2GetBackingStoreFormat(int *format,
> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> index 596746ccb7..c84e013f46 100644
> --- a/src/util/virstoragefile.h
> +++ b/src/util/virstoragefile.h
> @@ -82,8 +82,7 @@ typedef enum {
>       * libvirt support for following a backing chain */
>      VIR_STORAGE_FILE_BACKING,
> 
> -    VIR_STORAGE_FILE_COW = VIR_STORAGE_FILE_BACKING,
> -    VIR_STORAGE_FILE_QCOW,
> +    VIR_STORAGE_FILE_QCOW = VIR_STORAGE_FILE_BACKING,
>      VIR_STORAGE_FILE_QCOW2,
>      VIR_STORAGE_FILE_QED,
>      VIR_STORAGE_FILE_VMDK,

Removing this from the XML schema, is akin to deleting an enum
entry from the public API. This is not something we can do, even if
we think it is unlikely to be used.

It is of course fine if we generate an error in drivers when trying to use
it at runtime though. 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RFC] storage: Remove 'cow' disk format
Posted by Peter Krempa 6 years, 1 month ago 
On Fri, Mar 23, 2018 at 11:37:17 +0000, Daniel Berrange wrote:
> On Fri, Mar 23, 2018 at 12:32:29PM +0100, Peter Krempa wrote:
> > There's no tests for this format and nothing seems to specifically care
> > about this format. QEMU does not even recognize it. Remove it completely.
> 
> This format is from the User Mode Linux driver, not QEMU.

Hmmm. I wanted to delete the UML driver too, but that was slightly more
work and it also looked like it might be possible to build it even now,
but I did not manage to do it.

> 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> >  docs/schemas/storagecommon.rng |  1 -
> >  src/util/virstoragefile.c      | 31 +------------------------------
> >  src/util/virstoragefile.h      |  3 +--
> >  3 files changed, 2 insertions(+), 33 deletions(-)

[...]

> > diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> > index 596746ccb7..c84e013f46 100644
> > --- a/src/util/virstoragefile.h
> > +++ b/src/util/virstoragefile.h
> > @@ -82,8 +82,7 @@ typedef enum {
> >       * libvirt support for following a backing chain */
> >      VIR_STORAGE_FILE_BACKING,
> > 
> > -    VIR_STORAGE_FILE_COW = VIR_STORAGE_FILE_BACKING,
> > -    VIR_STORAGE_FILE_QCOW,
> > +    VIR_STORAGE_FILE_QCOW = VIR_STORAGE_FILE_BACKING,
> >      VIR_STORAGE_FILE_QCOW2,
> >      VIR_STORAGE_FILE_QED,
> >      VIR_STORAGE_FILE_VMDK,
> 
> Removing this from the XML schema, is akin to deleting an enum
> entry from the public API. This is not something we can do, even if
> we think it is unlikely to be used.

Yes I know. But given the chance to delete unused code I had to try it.
Especially since it was easy and I doubt that anyone would ever care.

> It is of course fine if we generate an error in drivers when trying to use
> it at runtime though. 

Yes. This is the patch I was doing after sending this.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.