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

[Ruslan Kuprieiev]

On 01/13/2015 08:10 PM, Pavel Emelyanov wrote:
>>> Is it technically possible to make the json text look like this:
>>>
>>>    {
>>>         "magic": "REG_FILES",
>>>         "entries": [
>>>             {
>>>                 "foo": "1",
>>>                 "bar": "BAR",
>>>                 "extra": {
>>>                       /* entry-specific stuff here */
>>>                 }
>>>             },
>>>
>>> ?
>>>
>>> And how bad would the code look like for this?
>> You mean like add "extra" field to the payload, right?
>> Yes, it does look better.
> Not exactly. I mean no "payload" keyword in the output, just
> the "extra" one for those with extra data (pipe-data, IPC).

Ok, will fix.

So, are you okay with names being tossed around if using json?

>>>>>> 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\""
>>>> Yes, we can, but it is a next step, i'm working on it in whole context
>>>> of impoving pb<->json convertation.
>>> This step should be accomplished till v1.5 (beginning of March). Can we?
>> Yes, definitely.
>> But I'll try to fix it asap.
> No hurry :) This is why I asked to cleanup the stuff incrementally
> AFTER which I will apply all the set.

Ok.

>>>> Hm... But what if we just store protobuf-text format string inside json?
>>> I don't understand this. How would the text look like?
>> Here is how reg-files.img would look like:
>>
>>
>> {
>>        "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"
>>            },
>>
>>
>> So the payload is a string that contains pb entry in protobuf text format.
> Ah, I see. No, this is not good. Let's choose only ONE format and
> print everything in it. Be it PB-test, JSON or even YAML doesn't
> matter. But the whole output should be in one format.
>

Ok.

>>>> It would solve all those problems with convertation and saving the names
>>>> order, but whould brake integrity check(which is still weak compared to
>>>> protobuf)...
>>>>
>>>> I was also considering packing image into one protobuf message,
>>>> similar to current json, when it looks like:
>>>> message criu_image {
>>>>       required string magic = 1;
>>>>       repeated entry entries = 2;
>>>> }
>>>>
>>>> message criu_entry {
>>>>       required criu_payload payload = 1;
>>>>       optional criu_extra extra = 2;
>>>> }
>>>>
>>>> message criu_payload {
>>>>       required string name = 1; // for example "core"
>>>>       optional core_entry core = 1;
>>>>       ....
>>>> }
>>>>
>>>> message criu_extra {
>>>>       required string name = 1; // for example "pagemaps"
>>>>       repeated pagemap_entry pagemaps = 2;
>>>>       ...
>>>> }
>>> PB docs say, that the format is good for encoding "small" messages (without
>>> specification of how small the small is). According to this, keeping all
>>> that CRIU generates in one entry (with sub-entries) is not good idea.
>> Yes, but we are using it just to print the images. I mean, we will read them
>> from *.img, pack in criu_image structure and print using pb text format.
>> And our images don't usually contain large amounts of data.
> Ah I see. Well, the temporary internal representation of data being
> de-/encoded is not really important, at least at this point.
>>>>>>> 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.
>>>> But wiki says that " Images in PB format are followed by zero or more entries of the _SAME TYPE_",
>>> Wiki is wrong here :) The EXTRA is stated to be "arbitrary blob", but it's
>>> not such in this case.
>> Why? It fits perfectly if we state that pagemap_entry's are the extra
>> for pagemap_head.
> Well, _if_ -- yes. But these are individual entries. So it's more
> correct just to treat this file of the image containing objects of
> different types. Which is kinda unique to how we planned to do images,
> but somehow unavoidable.

Oh, I see. Will fix.

>>>> which makes perfect sense to me. As I mentioned before, wiki description looks a
>>>> lot better than linear.
>>> The images we generate are not nice, yes. We even have a page describing how
>>> exactly they are not nice. But this is what we have, and starting developing
>>> new format is good task, but too early. Let's try to make crit decode produce
>>> as nice-looking output as can possibly be with the current images set.
>> Those messages (criu_image, criu_payload etc) are not for changing our
>> images =).
>> They are just to contain and treat images within crit, just to keep them
>> sorted and
>> nice-looking and be able to just parse human readable image using standart
>> ParseFromString protobuf method=).
>>
>>> Thanks,
>>> Pavel
>>>
>> .
>>