[CRIU] [PATCH 0/7] CRiu Image Tool(crit), v4

Tue Jan 13 05:29:33 PST 2015

On 01/12/2015 07:42 PM, Ruslan Kuprieiev wrote:
> On 01/12/2015 05:06 PM, Pavel Emelyanov wrote:
>> OK, I like what is there in the set :) Let's do a set of incremental changes
>> and I'll apply the set. Or, if you prefer, just rework the whole set. So.
>>
>> 1. In order to print the contents of the file on the screen in
>> human readable format the CLI I should use is
>>
>> # ./crit convert -i test/dump/static/env00/4300/1/reg-files.img
>>
>> This is strange, as "convert" generally means converting from one
>> format _to_ _another_ and in this case we always convert to json and
>> no other format.
>>
>>  From my perspective we should either rename action name from convert to
>> "show", "decode" or smth like this or require the target format to be
>> explicitly specified. The former is more preferable from my perspective.
>>
> 
> But we discussed previously that we should use stdin and stdout as
> defaults. We can use "show" name to print contents to the stdout, but
> "show" will have no sense when converting back.
> 
> Maybe you meant that "show" should be an alias for "convert -i"?
> If so, that sounds good.
> 
> "Decode"/"encode" doesn't fit here, as we have a format detection
> implemented.
> 
> Specifying target format looks redundant, as we have only 2 formats:
> binary and json(i'm not sure we need to convert to any other type, as
> there are a hell lot of tools to convert json to anything =)). So we
> can just convert input to the opposite format.

:) Yes, I understand. But I still want to see some CLI that, well, "makes
sense" in a certain meaning.

How about the following?

# crit decode [-i protobuf file, stdin by default] [-o output file, stdout by default]

takes input file, checks it being the criu PB one and decodes one into json.

# crit encode [-i json stream, stdin by default] [-o output file, stdount by default]

takes input json stream, checking being such and encodes one back to criu PB.

>> 2. Let's look at the output of inventory.img
>>
>> {
>>      "magic": "INVENTORY",
>>      "entries": [
>>          {
>>              "payload": {
>>                  "root_cg_set": "1",
>>                  "ns_per_id": "true",
>>                  "fdinfo_per_id": "true",
>>                  "root_ids": {
>>                      "sighand_id": "1",
>>                      "pid_ns_id": "1",
>>                      "uts_ns_id": "4",
>>                      "net_ns_id": "2",
>>                      "files_id": "1",
>>                      "user_ns_id": "6",
>>                      "ipc_ns_id": "3",
>>                      "fs_id": "1",
>>                      "mnt_ns_id": "5",
>>                      "vm_id": "1"
>>                  },
>>                  "img_version": "1"
>>              }
>>          }
>>      ]
>> }[root at localhost criu]#
>>
>> I have the following concerns about it.
>>
>> a) No newline after the last }
>> b) The "payload" field is obfuscating, there's no such thing in the pb images.
>> c) Why all the values are in ""-s?
>>
> 
> a) Oh, yes, we can easily add new line when printing to the stdout.
>     Will fix.

Thanks.

> b) About payload and extra fields. I think we need them, as they
>     show the structure of criu images according to one that is
>     described in our Images wiki page:
> 
> All criu-specific image files begin with 32-bit magic cookie. Images in
> PB format are followed by zero or more entries of the same type (not
> size!), each entry is preceded with 32-bit entry size value (not
> including this 32-bit value itself). Optionally each entry may be
> followed by extra payload which depends on the entry type.
> 
> IOW protocol-buffers image files look like
> 
> IMAGE_FILE ::= MAGIC { ENTRY }
> ENTRY      ::= SIZE PAYLOAD [ EXTRA ]
> PAYLOAD    ::= "message encoded in ProtocolBuffer format"
> EXTRA      ::= "arbitrary blob, depends on the PAYLOAD contents"
> 
> MAGIC      ::= "32 bit integer"
> SIZE       ::= "32 bit integer, equals the PAYLOAD length"
> 
>     So the current structure with "payload" and "extra" fields
>     makes perfect sense to me. The linear structure that
>     we've used previously in our text-format confuses the user,
>     just the way I was confused about pagemap\pipes-data image
>     structure.

Hm... The wiki page is not a rule written in stone. Let's make the
json representation look human-readable and fix the wiki page. Like
this:

IMAGE_FILE ::= MAGIC { SIZE ENTRY [PAYLOAD] }
ENTRY      ::= "message encoded in PB"
PAYLOAD    ::= "arbitrary blob"

:)

Too many braces and extra fields make things harder to understand.

> c) I wrap every value in "" as it isn't that easy to convert
>     from pb to json, as theirs supported types do not match,
>     and wrapping everything into strings is just the easiest
>     way to get everything work. I'm currently thinking on
>     a way to improve that, but for now it works just fine.

OK. Can we at least keep strings as string, not as string with string?
I mean this:

"path": "/foo/bar"

instead of

"path": "\"/foo/bar\""

>> 3. Let's now look at reg-files.img
>>
>> {
>>      "magic": "REG_FILES",
>>      "entries": [
>>          {
>>              "payload": {
>>                  "fown": {
>>                      "pid_type": "0",
>>                      "signum": "0",
>>                      "pid": "0",
>>                      "uid": "0",
>>                      "euid": "0"
>>                  },
>>                  "flags": "32770",
>>                  "id": "1",
>>                  "name": "\"/dev/null\"",
>>                  "pos": "0"
>>              }
>>          },
>>          {
>>              "payload": {
>>                  "name": "\"/\"",
>>                  "fown": {
>>                      "pid_type": "0",
>>                      "signum": "0",
>>                      "pid": "0",
>>                      "uid": "0",
>>                      "euid": "0"
>>                  },
>>                  "pos": "0",
>>                  "flags": "33793",
>>                  "id": "2",
>>                  "size": "0"
>>              }
>>          },
>>
>> Other than those from inventory, the concerns here are:
>>
>> a) The order of fields differs from entry to entry.
> 
> It is normal for json =).

Yes, but not for human eyes :)

> I thought about it and concluded that it doesn't harm us in any way,
> as we only care about the order in "repeated" fields and we treat
> them properly by converting to an array, so the order is saved as
> the order of elements in json arrays is preserved.

How hard would it be to keep names in the order they are met in pb file?

>> b) The name of the 2nd entry is just /, but in the output it takes 7 characters to print one.
>>
> 
> Yep, that is a side-effect of that simplified pb<->json convertation.
> 
>> 4. Next the pagemap.img
>>
>> {
>>      "magic": "PAGEMAP",
>>      "entries": [
>>          {
>>              "payload": {
>>                  "pages_id": "1"
>>              },
>>              "extra": [
>>                  {
>>                      "nr_pages": "1",
>>                      "vaddr": "4194304"
>>                  },
>>
>> Similar to the inventory, field "extra" is obfuscating.
>>
> 
> See 1.

Disagree. The pagemap_entry-s are not "extra". They are entries w/o extra payload.

>> 5. And the last one is pages.img. The output is
>>
>> Traceback (most recent call last):
>>    File "./crit", line 76, in <module>
>>      main()
>>    File "./crit", line 73, in main
>>      cmds[opts['command']](opts)
>>    File "./crit", line 45, in convert_img
>>      img = pycriu.images.loads(in_str)
>>    File "/root/src/criu/pycriu/images/images.py", line 271, in loads
>>      return load(f)
>>    File "/root/src/criu/pycriu/images/images.py", line 254, in load
>>      raise Exception("Unknown magic "+str(img_magic))
>> Exception: Unknown magic 1179403647
>>
>> Can we catch this exception and print something meaningful?
>>
> 
> Yes, we can. Will fix.

Cool :)

Thanks,
Pavel