| Message ID | 1498733831-15254-2-git-send-email-pl@kamp.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben: > Signed-off-by: Peter Lieven <pl@kamp.de> > --- > docs/interop/qcow2.txt | 43 ++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 42 insertions(+), 1 deletion(-) > > diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt > index 80cdfd0..c01daf3 100644 > --- a/docs/interop/qcow2.txt > +++ b/docs/interop/qcow2.txt > @@ -85,7 +85,12 @@ in the description of a field. > be written to (unless for regaining > consistency). > > - Bits 2-63: Reserved (set to 0) > + Bit 2: Compress format bit. If and only if this bit > + is set then the compress format extension > + MUST be present and MUST be parsed and checked > + for compatibility. > + > + Bits 3-63: Reserved (set to 0) > > 80 - 87: compatible_features > Bitmask of compatible features. An implementation can > @@ -135,6 +140,7 @@ be stored. Each extension has a structure like the following: > 0xE2792ACA - Backing file format name > 0x6803f857 - Feature name table > 0x23852875 - Bitmaps extension > + 0xC03183A3 - Compress format extension > other - Unknown header extension, can be safely > ignored > > @@ -208,6 +214,41 @@ The fields of the bitmaps extension are: > starts. Must be aligned to a cluster boundary. > > > +== Compress format extension == > + > +The compress format extension is an optional header extension. It provides > +the ability to specify the compress algorithm and compress parameters > +that are used for compressed clusters. This new header MUST be present if > +the incompatible-feature bit "compress format bit" is set and MUST be absent > +otherwise. Okay. Only one header extension type for all compression formats. I think this does work because we have a variable header extension size. > +The fields of the compress format extension are: > + > + Byte 0 - 15: compress_format_name (padded with zeros, but not > + necessarily null terminated if it has full length) We absolutely need to define the valid names and their meaning here. > + 16: compress_level (uint8_t) > + 0 = default compress level > + 1 = lowest compress level > + x = highest compress level (the highest compress > + level may vary for different compress formats) Let's be explicit about the compression formats that this field is valid for (i.e. make this already part of the format specific fields). Then we can also be specific instead of writing "may vary", which is not a very good thing to have in a specification. > + 17 - 19: Reserved for future use, must be zero. We don't need this for now because byte 16 will be the final one in this struct. > + 20 - 23: extra_data_size > + Size of compress format specific extra data. > + For now, as no format specifies extra data, > + extra_data_size is reserved and should be zero. > + > + variable: extra_data > + Extra data with additional parameters for the compress > + format, occupying extra_data_size bytes. extra_data_size isn't necessary because we already have the length of the header extension, so we know how long the whole thing is. extra_data isn't necessary because we'll just add new fields at the end if we want to add something. > + variable: Padding to round up the size of compress format extension > + to the next multiple of 8. All bytes of the padding must be > + zero. This is already contained in the description of header extensions in general, so we don't have to repeat it here. Kevin
Am 10.07.2017 um 14:58 schrieb Kevin Wolf: > Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben: >> Signed-off-by: Peter Lieven <pl@kamp.de> >> --- >> docs/interop/qcow2.txt | 43 ++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 42 insertions(+), 1 deletion(-) >> >> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt >> index 80cdfd0..c01daf3 100644 >> --- a/docs/interop/qcow2.txt >> +++ b/docs/interop/qcow2.txt >> @@ -85,7 +85,12 @@ in the description of a field. >> be written to (unless for regaining >> consistency). >> >> - Bits 2-63: Reserved (set to 0) >> + Bit 2: Compress format bit. If and only if this bit >> + is set then the compress format extension >> + MUST be present and MUST be parsed and checked >> + for compatibility. >> + >> + Bits 3-63: Reserved (set to 0) >> >> 80 - 87: compatible_features >> Bitmask of compatible features. An implementation can >> @@ -135,6 +140,7 @@ be stored. Each extension has a structure like the following: >> 0xE2792ACA - Backing file format name >> 0x6803f857 - Feature name table >> 0x23852875 - Bitmaps extension >> + 0xC03183A3 - Compress format extension >> other - Unknown header extension, can be safely >> ignored >> >> @@ -208,6 +214,41 @@ The fields of the bitmaps extension are: >> starts. Must be aligned to a cluster boundary. >> >> >> +== Compress format extension == >> + >> +The compress format extension is an optional header extension. It provides >> +the ability to specify the compress algorithm and compress parameters >> +that are used for compressed clusters. This new header MUST be present if >> +the incompatible-feature bit "compress format bit" is set and MUST be absent >> +otherwise. > Okay. Only one header extension type for all compression formats. I > think this does work because we have a variable header extension size. > >> +The fields of the compress format extension are: >> + >> + Byte 0 - 15: compress_format_name (padded with zeros, but not >> + necessarily null terminated if it has full length) > We absolutely need to define the valid names and their meaning here. > >> + 16: compress_level (uint8_t) >> + 0 = default compress level >> + 1 = lowest compress level >> + x = highest compress level (the highest compress >> + level may vary for different compress formats) > Let's be explicit about the compression formats that this field is valid > for (i.e. make this already part of the format specific fields). > > Then we can also be specific instead of writing "may vary", which is not > a very good thing to have in a specification. The idea to keep it in the common header was that most formats will only need a compression level and nothing more. So it is not necessary to add specific headers for a setting that is quite common among compression formats. > >> + 17 - 19: Reserved for future use, must be zero. > We don't need this for now because byte 16 will be the final one in this > struct. > >> + 20 - 23: extra_data_size >> + Size of compress format specific extra data. >> + For now, as no format specifies extra data, >> + extra_data_size is reserved and should be zero. >> + >> + variable: extra_data >> + Extra data with additional parameters for the compress >> + format, occupying extra_data_size bytes. > extra_data_size isn't necessary because we already have the length of > the header extension, so we know how long the whole thing is. > extra_data isn't necessary because we'll just add new fields at the end > if we want to add something. I was asked to add it altough it is redundant. > >> + variable: Padding to round up the size of compress format extension >> + to the next multiple of 8. All bytes of the padding must be >> + zero. > This is already contained in the description of header extensions in > general, so we don't have to repeat it here. Its also in the bitmap extension... Peter
Am 10.07.2017 um 15:27 hat Peter Lieven geschrieben: > Am 10.07.2017 um 14:58 schrieb Kevin Wolf: > >Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben: > >>Signed-off-by: Peter Lieven <pl@kamp.de> > >>--- > >> docs/interop/qcow2.txt | 43 ++++++++++++++++++++++++++++++++++++++++++- > >> 1 file changed, 42 insertions(+), 1 deletion(-) > >> > >>diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt > >>index 80cdfd0..c01daf3 100644 > >>--- a/docs/interop/qcow2.txt > >>+++ b/docs/interop/qcow2.txt > >>@@ -85,7 +85,12 @@ in the description of a field. > >> be written to (unless for regaining > >> consistency). > >>- Bits 2-63: Reserved (set to 0) > >>+ Bit 2: Compress format bit. If and only if this bit > >>+ is set then the compress format extension > >>+ MUST be present and MUST be parsed and checked > >>+ for compatibility. > >>+ > >>+ Bits 3-63: Reserved (set to 0) > >> 80 - 87: compatible_features > >> Bitmask of compatible features. An implementation can > >>@@ -135,6 +140,7 @@ be stored. Each extension has a structure like the following: > >> 0xE2792ACA - Backing file format name > >> 0x6803f857 - Feature name table > >> 0x23852875 - Bitmaps extension > >>+ 0xC03183A3 - Compress format extension > >> other - Unknown header extension, can be safely > >> ignored > >>@@ -208,6 +214,41 @@ The fields of the bitmaps extension are: > >> starts. Must be aligned to a cluster boundary. > >>+== Compress format extension == > >>+ > >>+The compress format extension is an optional header extension. It provides > >>+the ability to specify the compress algorithm and compress parameters > >>+that are used for compressed clusters. This new header MUST be present if > >>+the incompatible-feature bit "compress format bit" is set and MUST be absent > >>+otherwise. > >Okay. Only one header extension type for all compression formats. I > >think this does work because we have a variable header extension size. > > > >>+The fields of the compress format extension are: > >>+ > >>+ Byte 0 - 15: compress_format_name (padded with zeros, but not > >>+ necessarily null terminated if it has full length) > >We absolutely need to define the valid names and their meaning here. > > > >>+ 16: compress_level (uint8_t) > >>+ 0 = default compress level > >>+ 1 = lowest compress level > >>+ x = highest compress level (the highest compress > >>+ level may vary for different compress formats) > >Let's be explicit about the compression formats that this field is valid > >for (i.e. make this already part of the format specific fields). > > > >Then we can also be specific instead of writing "may vary", which is not > >a very good thing to have in a specification. > > The idea to keep it in the common header was that most formats will > only need a compression level and nothing more. So it is not necessary > to add specific headers for a setting that is quite common among > compression formats. I'm only talking about a change in the specification text, which would give us a clean separation design-wise and a more specific explanation. We have regretted enough things that were defined a bit sloppily just for convenience that I'd rather not make this mistake again when we can avoid it. > >>+ 17 - 19: Reserved for future use, must be zero. > >We don't need this for now because byte 16 will be the final one in this > >struct. > > > >>+ 20 - 23: extra_data_size > >>+ Size of compress format specific extra data. > >>+ For now, as no format specifies extra data, > >>+ extra_data_size is reserved and should be zero. > >>+ > >>+ variable: extra_data > >>+ Extra data with additional parameters for the compress > >>+ format, occupying extra_data_size bytes. > >extra_data_size isn't necessary because we already have the length of > >the header extension, so we know how long the whole thing is. > >extra_data isn't necessary because we'll just add new fields at the end > >if we want to add something. > > I was asked to add it altough it is redundant. Who asked this, and did they have a good reason for it? > > > >>+ variable: Padding to round up the size of compress format extension > >>+ to the next multiple of 8. All bytes of the padding must be > >>+ zero. > >This is already contained in the description of header extensions in > >general, so we don't have to repeat it here. > > Its also in the bitmap extension... The bitmap header extension is aligned, so it doesn't have padding. Some other bitmap structures need padding, but they are not header extensions, so the information isn't redundant for them Kevin
On 07/10/2017 08:50 AM, Kevin Wolf wrote: > We have regretted enough things that were defined a bit sloppily just > for convenience that I'd rather not make this mistake again when we can > avoid it. Agreed. Another consideration: defining redundant information is trickier to maintain because correct implementations have to check that both points are consistent with one another. > >>>> + 17 - 19: Reserved for future use, must be zero. >>> We don't need this for now because byte 16 will be the final one in this >>> struct. >>> >>>> + 20 - 23: extra_data_size >>>> + Size of compress format specific extra data. >>>> + For now, as no format specifies extra data, >>>> + extra_data_size is reserved and should be zero. >>>> + >>>> + variable: extra_data >>>> + Extra data with additional parameters for the compress >>>> + format, occupying extra_data_size bytes. >>> extra_data_size isn't necessary because we already have the length of >>> the header extension, so we know how long the whole thing is. >>> extra_data isn't necessary because we'll just add new fields at the end >>> if we want to add something. >> >> I was asked to add it altough it is redundant. > > Who asked this, and did they have a good reason for it? May have been me, or may have been implied by my comments, but I agree that the overall extension header covers this, so we don't need a redundant form. So as long as the overall extension header length is good enough, I'm okay avoiding redundancies.
diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt index 80cdfd0..c01daf3 100644 --- a/docs/interop/qcow2.txt +++ b/docs/interop/qcow2.txt @@ -85,7 +85,12 @@ in the description of a field. be written to (unless for regaining consistency). - Bits 2-63: Reserved (set to 0) + Bit 2: Compress format bit. If and only if this bit + is set then the compress format extension + MUST be present and MUST be parsed and checked + for compatibility. + + Bits 3-63: Reserved (set to 0) 80 - 87: compatible_features Bitmask of compatible features. An implementation can @@ -135,6 +140,7 @@ be stored. Each extension has a structure like the following: 0xE2792ACA - Backing file format name 0x6803f857 - Feature name table 0x23852875 - Bitmaps extension + 0xC03183A3 - Compress format extension other - Unknown header extension, can be safely ignored @@ -208,6 +214,41 @@ The fields of the bitmaps extension are: starts. Must be aligned to a cluster boundary. +== Compress format extension == + +The compress format extension is an optional header extension. It provides +the ability to specify the compress algorithm and compress parameters +that are used for compressed clusters. This new header MUST be present if +the incompatible-feature bit "compress format bit" is set and MUST be absent +otherwise. + +The fields of the compress format extension are: + + Byte 0 - 15: compress_format_name (padded with zeros, but not + necessarily null terminated if it has full length) + + 16: compress_level (uint8_t) + 0 = default compress level + 1 = lowest compress level + x = highest compress level (the highest compress + level may vary for different compress formats) + + 17 - 19: Reserved for future use, must be zero. + + 20 - 23: extra_data_size + Size of compress format specific extra data. + For now, as no format specifies extra data, + extra_data_size is reserved and should be zero. + + variable: extra_data + Extra data with additional parameters for the compress + format, occupying extra_data_size bytes. + + variable: Padding to round up the size of compress format extension + to the next multiple of 8. All bytes of the padding must be + zero. + + == Host cluster management == qcow2 manages the allocation of host clusters by maintaining a reference count
Signed-off-by: Peter Lieven <pl@kamp.de> --- docs/interop/qcow2.txt | 43 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-)