Binary encode

[bmarzins]

This patchset adds support for dumping udev variables in sidctl, and printing binary dump data in base64 encoding. A number of the changes for encoding the base64 data are there to avoid mallocing memory for the encoded data, which would just be freed after writing it to the memfd buffer. If you don't care about the extra malloc for binary data, this patchset can get simplified.

[bmarzins]

Um... The only output I can find for the failed tests is from an apparently successful configuration. Does anyone know how to see what actually failed.

[prajnoha]

Um... The only output I can find for the failed tests is from an apparently successful configuration. Does anyone know how to see what actually failed.

Hmm, try again now - if you click on the "Details" next to the failed "Complete Jenkins Job (private)" here on github, then you get to the Jenkins page and in the "Configuration" there's "distro=f_33 distro=centos_stream distro=f_rawhide". If you click on one of them, then "Console Output" (on the left banner), then there's:

+ sudo /usr/local/sbin/sidctl dump --format json
<sidctl> Failed to map memory with key-value store: Invalid argument.
<sidctl> Failed to unmap memory with key-value store: Invalid argument.
Build step 'Execute shell' marked build as failure

But I'm not sure why - I can run that just fine on my machine... hmm..

[prajnoha]

Regarding the buffer_add with NULL data - usually I'm trying to avoid a situation where we get the memory out of buffer and then we write the data directly to the that. I have to admit that at the very beginning, I wanted to keep this memory only writeable by buffer code itself and keep the memory outside the buffer strictly read-only so we can always trace the changes to the memory and always check the limits inside buffer code (to avoid overflows etc). But I understand that in certain cases we need it writeable outside (...mainly to avoid uselessly more complex solutions).

Would be great if we could add a custom format specifier for buffer_fmt_add which would mean "translate to base64" (or "do not add trailing \0 at the end of string like we needed in some other situation before). I found out that it's possible to extend the printf (https://www.gnu.org/software/libc/manual/html_node/Customizing-Printf.html), but it's not standard and applies only to glibc so we probably can't make reliable use of that(...and I haven't tried that so I'm not even sure if that can be used for other printf-like functions like snprintf).

An alternative would be to simply add buffer_base64_write fn to buffer interface, but we could be adding such "format" write fns for any other format we'd need anytime later and that could be messy in the end.

And I can't think of any other sane alternative right now... So ok, let's got with the solution as you provided, it's probably the least problematic/disputable one.

(With vector buffers it's a bit different because we're adding the iovecs, and the data is completely external.)

The compiler just fusses a bit about this, but otherwise the patches look good:
formatter.c:147:31: warning: pointer targets in passing argument 1 of ‘_print_binary’ differ in signedness [-Wpointer-sign]

Thanks!

[bmarzins]

Well. the cause of the unmap failure is obvious. The mmap failure is less so. If the length was 0, that would cause an EINVAL failure according the the man page. However, there should have been some things in the database, unless perhaps the request came in too soon (I'm not sure if that can even happen). Maybe there was an error on the server size. I'll look into it some more. At any rate, if nothing has been added to the buffer before buffer_get_fd() is called on prefixed buffers, it needs to change the size from 0 to MSG_SIZE_PREFIX_LEN.

[prajnoha]

At its start, the db has only reserved keys in it. In case we're not building blkid module (I've noticed gperf is missing which is a dependency for this module) and also mpath module, then the db doesn't have anything in it. So that might be the reason indeed!

[bmarzins]

Regarding the buffer_add with NULL data - usually I'm trying to avoid a situation where we get the memory out of buffer and then we write the data directly to the that. I have to admit that at the very beginning, I wanted to keep this memory only writeable by buffer code itself and keep the memory outside the buffer strictly read-only so we can always trace the changes to the memory and always check the limits inside buffer code (to avoid overflows etc). But I understand that in certain cases we need it writeable outside (...mainly to avoid uselessly more complex solutions).

Would be great if we could add a custom format specifier for buffer_fmt_add which would mean "translate to base64" (or "do not add trailing \0 at the end of string like we needed in some other situation before). I found out that it's possible to extend the printf (https://www.gnu.org/software/libc/manual/html_node/Customizing-Printf.html), but it's not standard and applies only to glibc so we probably can't make reliable use of that(...and I haven't tried that so I'm not even sure if that can be used for other printf-like functions like snprintf).

An alternative would be to simply add buffer_base64_write fn to buffer interface, but we could be adding such "format" write fns for any other format we'd need anytime later and that could be messy in the end.

And I can't think of any other sane alternative right now... So ok, let's got with the solution as you provided, it's probably the least problematic/disputable one.

Like I mentioned in the pull request message, if you are willing to have base64_encode() alloc some memory that we copy to the buffer and then free, then we don't need alloc empty memory. It's just a question of whether avoiding writing to buffer memory from outside the buffer code is worth the extra malloc. I'm fine with either way.

(With vector buffers it's a bit different because we're adding the iovecs, and the data is completely external.)

The compiler just fusses a bit about this, but otherwise the patches look good:
formatter.c:147:31: warning: pointer targets in passing argument 1 of ‘_print_binary’ differ in signedness [-Wpointer-sign]

I'll fix that.

Thanks!

[trgill]

FYI - you are welcome to use the Jenkins node to reproduce the results. We just need to take it offline/put it back in the pool. I think you've got permission to do that - but I'm happy to help organize etc.

[bmarzins]

So to fix the "Failed to map memory with key-value store: Invalid argument." error, my plan is to go through the code and make sure that we check if buf->stat.usage.used == 0 on size-prefixed buffers and update it to count the prefix size, in all the places where we need to do that. There is one place were we don't need to update it, but I think we might want to. That's in buffer_write().

Currently, buffer_write_all() intentionally doesn't write anything, even the size prefix, if a prefixed buffer is empty. However, the callers of buffer_read() treat reading a complete empty buffer as different than reading a complete buffer of any other size. This is fine, since the callers of read_buffer() can't do anything with an empty buffer anyway, but it seems more sensible for the writers to still write the size prefix for empty buffers, and reads to treat them as having read a complete buffer. Obviously the readers will then need to check if they got the buffer size that they were expecting, which they don't currently do. This is a bug. They shouldn't just assume that because the buffer they got is non-zero, that it is the size they are expecting, and start accessing memory without checking. And since we need to check that the complete buffers we read are the size we were expecting anyways, I don't see why a complete buffer with no data should be treated any differently than any other complete buffer with the wrong size of data.

Let me know if you think I should change the write_buffer() behavior as well.

[prajnoha]

Like I mentioned in the pull request message, if you are willing to have base64_encode() alloc some memory that we copy to the buffer and then free, then we don't need alloc empty memory. It's just a question of whether avoiding writing to buffer memory from outside the buffer code is worth the extra malloc. I'm fine with either way.

Let's stay with your current patch - probably better to avoid the extra malloc rather than being so strict about buffers...

So to fix the "Failed to map memory with key-value store: Invalid argument." error, my plan is to go through the code and make sure that we check if buf->stat.usage.used == 0 on size-prefixed buffers and update it to count the prefix size, in all the places where we need to do that. There is one place were we don't need to update it, but I think we might want to. That's in buffer_write().

OK. For linear buffers, that's clear... But for vector buffers, we need to take care that the buf->stat.usage metrics are in "number of iovec structs" units, not the "memory used in bytes" units. When the vector buffer is written (using writev), we calculate the actual size in bytes by counting sizes of all iovec.iov_len in the vector so there's the real "memory used in bytes" prefix used. For the write, if we have used == 0, we should be skipping the write completely - it returns -ENODATA.

The intention was to update the size prefix only when needed, that is right before writing out the buffer. So either before writeor get_fd where we expect we're going to send the fd somewhere. From this perspective, maybe the get_fd should be returning -ENODATA in case there's no data written yet in the buffer.

Currently, buffer_write_all() intentionally doesn't write anything, even the size prefix, if a prefixed buffer is empty. However, the callers of buffer_read() treat reading a complete empty buffer as different than reading a complete buffer of any other size.

Hmm, but the reader shouldn't see empty data (unless there's a problem during read). The write end only writes if there are data to write, otherwise skips (the -ENODATA I mentioned above). Of course, this assumes that the buffer interface is used on both write and read end (at least for BUFFER_MODE_SIZE_PREFIX). It's true that we can't assure that it's used that way in all cases... We'd need a more elaborate protocol for that.

This is fine, since the callers of read_buffer() can't do anything with an empty buffer anyway, but it seems more sensible for the writers to still write the size prefix for empty buffers, and reads to treat them as having read a complete buffer. Obviously the readers will then need to check if they got the buffer size that they were expecting, which they don't currently do. This is a bug.

Yeah, could be done that way too. I just wanted to avoid any writes if there's no real data to write, just like it is with BUFFER_MODE_PLAIN, so I wanted to keep the interface working exactly the same for both BUFFER_MODE_SIZE_PREFIX and PLAIN. The prefix mode only adding the extra prefix transparently.

As for checking that we got the amount of data as given by the prefix, that should be covered by:

sid/src/base/buffer-type-linear.c

Lines 339 to 341 in 0370661

	expected = EXPECTED(buf);
	if ((!expected && buf->stat.usage.used) || (expected && buf->stat.usage.used != expected))
	return -EBADMSG;

Or did you mean a different check?

[bmarzins]

The intention was to update the size prefix only when needed, that is right before writing out the buffer. So either before writeor get_fd where we expect we're going to send the fd somewhere. From this perspective, maybe the get_fd should be returning -ENODATA in case there's no data written yet in the buffer.

I would personally prefer to always pass the fd, even if it's empty. But as long as we pass the fd, if you really don't like updating the size, we don't have to in this case, The receiver can still read the size prefix from the passed memfd. Whether we set it to 0 or MSG_SIZE_PREFIX_LEN, the receiver can know that if it's not larger than MSG_SIZE_PREFIX_LEN, the buffer is empty.

Currently, buffer_write_all() intentionally doesn't write anything, even the size prefix, if a prefixed buffer is empty. However, the callers of buffer_read() treat reading a complete empty buffer as different than reading a complete buffer of any other size.

Hmm, but the reader shouldn't see empty data (unless there's a problem during read). The write end only writes if there are data to write, otherwise skips (the -ENODATA I mentioned above). Of course, this assumes that the buffer interface is used on both write and read end (at least for BUFFER_MODE_SIZE_PREFIX). It's true that we can't assure that it's used that way in all cases... We'd need a more elaborate protocol for that.

What I'm talking about is when buffer_read() returns 0 bytes read (without an error), but buffer_is_complete() returns false. This is a complete but empty buffer (buffer_is_complete() will always return false for empty buffers), and callers of buffer_read() treat this differently then when they read a complete buffer of any other size.

This is fine, since the callers of read_buffer() can't do anything with an empty buffer anyway, but it seems more sensible for the writers to still write the size prefix for empty buffers, and reads to treat them as having read a complete buffer. Obviously the readers will then need to check if they got the buffer size that they were expecting, which they don't currently do. This is a bug.

Yeah, could be done that way too. I just wanted to avoid any writes if there's no real data to write, just like it is with BUFFER_MODE_PLAIN, so I wanted to keep the interface working exactly the same for both BUFFER_MODE_SIZE_PREFIX and PLAIN. The prefix mode only adding the extra prefix transparently.

Unless someone does buffer_add(buf, "", 0, NULL); which will leave you with an empty buffer that does write out the prefix.

Also, I'm just not sure that this gets us, except for extra code on the reader end to deal with the special case of empty prefixed buffers. The code to check if you read is a complete but empty buffer with a prefix either already needs to be there to check for complete but too small buffers, or already is there like here:

sid/src/resource/worker-control.c

Lines 252 to 260 in 352df2a

	(void) buffer_get_data(chan->in_buf, (const void **) &buf_data, &buf_data_size);
	/*
	* Internal workers and associated proxies use BUFFER_MODE_SIZE_PREFIX buffers and
	* they always transmit worker_channel_cmd_t as header before actual data.
	*/
	memcpy(cmd, buf_data, sizeof(*cmd));
	data_spec->data_size = buf_data_size - sizeof(*cmd);
	data_spec->data = data_spec->data_size > 0 ? buf_data + sizeof(*cmd) : NULL;

which already handles the case of an empty buffer where the prefix was written.

As for checking that we got the amount of data as given by the prefix, that should be covered by:

sid/src/base/buffer-type-linear.c

Lines 339 to 341 in 0370661

	expected = EXPECTED(buf);
	if ((!expected && buf->stat.usage.used) || (expected && buf->stat.usage.used != expected))
	return -EBADMSG;

Or did you mean a different check?

I was talking about:

sid/src/resource/ubridge.c

Lines 3629 to 3634 in 352df2a

	if (buffer_is_complete(conn->buf, NULL)) {
	(void) buffer_get_data(conn->buf, (const void **) &msg.header, &msg.size);
	/* Sanitize command number - map all out of range command numbers to CMD_UNKNOWN. */
	if (msg.header->cmd < _USID_CMD_START || msg.header->cmd > _USID_CMD_END)
	msg.header->cmd = USID_CMD_UNKNOWN;

If buffer_is_complete() returns true, the code also assumes that the buffer it read is large enough to hold a usid_msg_header. But just because the entire client buffer was sent, doesn't mean that the entire client buffer was large enough to hold a usid_msg_header. To avoid the possibility of reading off the end of the buffer, sid needs to check that msg.size is large enough before accessing msg.header. Checking if the buffer is empty isn't a special case if the prefix size is written for empty buffers. It just gets caught in the check for if the buffer is too small.

At any rate, this is your project. So if you want to leave buffer_write() and buffer_get_fd() the way they are, I'll do that.

[prajnoha]

What I'm talking about is when buffer_read() returns 0 bytes read (without an error), but buffer_is_complete() returns false. This is a complete but empty buffer (buffer_is_complete() will always return false for empty buffers), and callers of buffer_read() treat this differently then when they read a complete buffer of any other size.

Well, honestly, I'm still not quite sure about the issue here :) If we're expecting size-prefixed buffer on the read side, we can have one of these, which should cover all possibilities, I hope:

• the size prefix was not read completely and we got buffer_read==0 ==> error/bad message/missing prefix

• the size was read completely and we got buffer_read==0 for the rest ==> error/incomplete message (or the write end incorrectly sent empty buffer so there's just prefix, which is currently considered an error too)

• the size was read completely and we got buffer_read>0 (possibly several times), then buffer_read==0 at the end ==> OK if what we read matches what the prefix says ("EXPECTED") or error if the size read does not match EXPECTED

The only thing here is that the receiving ("read") side needs to know that we are expecting size-prefixed buffer. So the other end always needs to know what type of buffer to use - whether there should be a prefix or not.

Yeah, could be done that way too. I just wanted to avoid any writes if there's no real data to write, just like it is with BUFFER_MODE_PLAIN, so I wanted to keep the interface working exactly the same for both BUFFER_MODE_SIZE_PREFIX and PLAIN. The prefix mode only adding the extra prefix transparently.

Unless someone does buffer_add(buf, "", 0, NULL); which will leave you with an empty buffer that does write out the prefix.

Yeah, that's a bug if when looking at this from the perspective of "never sending empty buffers"!

Also, I'm just not sure that this gets us, except for extra code on the reader end to deal with the special case of empty prefixed buffers. The code to check if you read is a complete but empty buffer with a prefix either already needs to be there to check for complete but too small buffers, or already is there like here:

sid/src/resource/worker-control.c

Lines 252 to 260 in 352df2a

	(void) buffer_get_data(chan->in_buf, (const void **) &buf_data, &buf_data_size);
	/*
	* Internal workers and associated proxies use BUFFER_MODE_SIZE_PREFIX buffers and
	* they always transmit worker_channel_cmd_t as header before actual data.
	*/
	memcpy(cmd, buf_data, sizeof(*cmd));
	data_spec->data_size = buf_data_size - sizeof(*cmd);
	data_spec->data = data_spec->data_size > 0 ? buf_data + sizeof(*cmd) : NULL;

which already handles the case of an empty buffer where the prefix was written.

Ehm, that's probably a superfluous check, because the writer shouldn't be even sending the empty buffer - so it shouldn't happen that the size prefix says that the overall buffer size == the size of the prefix itself and nothing else (== no data) - in that case, the buffer shouldn't be even sent on the other ("write") side. (If by chance we're sending an "empty buffer" with no data, that is a bug then.)

I was talking about:

sid/src/resource/ubridge.c

Lines 3629 to 3634 in 352df2a

	if (buffer_is_complete(conn->buf, NULL)) {
	(void) buffer_get_data(conn->buf, (const void **) &msg.header, &msg.size);
	/* Sanitize command number - map all out of range command numbers to CMD_UNKNOWN. */
	if (msg.header->cmd < _USID_CMD_START || msg.header->cmd > _USID_CMD_END)
	msg.header->cmd = USID_CMD_UNKNOWN;

If buffer_is_complete() returns true, the code also assumes that the buffer it read is large enough to hold a usid_msg_header. But just because the entire client buffer was sent, doesn't mean that the entire client buffer was large enough to hold a usid_msg_header. To avoid the possibility of reading off the end of the buffer, sid needs to check that msg.size is large enough before accessing msg.header. Checking if the buffer is empty isn't a special case if the prefix size is written for empty buffers. It just gets caught in the check for if the buffer is too small.

Oh! Nice catch! Sure, we need to fix that, add the check, of course...

At any rate, this is your project. So if you want to leave buffer_write() and buffer_get_fd() the way they are, I'll do that.

Well, I see your point - the thing with trying to not send empty buffers and fiddling with the buf->stat.usage.used to be set only after at least one data addition to the buffer, might cause more issues than good (due to the extra checks we need to do there).

So if things get simplified and we have less potential for bugs with always setting the usage.used and counting even the size prefix itself right away, I'm fine with that. If we really don't want to send the empty buffer anywhere in the code, the caller can still do the check on its own right before it calls the buffer_write. The code for calling out buffers for MODE_PLAIN and MODE_SIZE_PREFIX would differ then a bit (or there will simply be a superfluous check for empty buffer for MODE_PLAIN because that one can check this inside buffer code directly), but it's not anything horrible. And as you say, even if we wrote the empty buffer, the reader would detect anyway...

So sure, if things will get simplified, go ahead! I just wanted to present what was meant with the original intention (to hide the difference between BUFFER_MODE_PLAIN and SIZE_PREFIX behing the buffer interface, but we probably can't hide it completely anyway).

[bmarzins]

Don't merge this yet. Regardless of whether or not we change how size prefixed write work, I have some cleanup that I notice I need to do on these commits.

There's also another issue that came up at the meeting today. David Lehman would prefer a library interface for getting information from SID. It seems like sidctl could just become a wrapper around this library. This makes one of the choices I made in my last pull request seem less correct, in hindsight. I didn't want to make the kv_store value formats part of the interface (by defining them in iface/usid.h). However, this meant that sidctl needed to link against libsidresource. It would also mean that anything that linked against this new libsidctl library would need to link against libsidresource. It seems like I might be better to simply define the value formats in usid.h, to keep libsidctl as simple as possible. If we end with a solution that doesn't involve exporting the worker changes using a memfd, then we can switch to using whatever format we want for the dump transfer. Thoughts?

[prajnoha]

Don't merge this yet. Regardless of whether or not we change how size prefixed write work, I have some cleanup that I notice I need to do on these commits.

OK.

There's also another issue that came up at the meeting today. David Lehman would prefer a library interface for getting information from SID. It seems like sidctl could just become a wrapper around this library. This makes one of the choices I made in my last pull request seem less correct, in hindsight. I didn't want to make the kv_store value formats part of the interface (by defining them in iface/usid.h). However, this meant that sidctl needed to link against libsidresource. It would also mean that anything that linked against this new libsidctl library would need to link against libsidresource. It seems like I might be better to simply define the value formats in usid.h, to keep libsidctl as simple as possible. If we end with a solution that doesn't involve exporting the worker changes using a memfd, then we can switch to using whatever format we want for the dump transfer. Thoughts?

Indeed, I somehow didn't realize fully that we actually created a completely new sid-tools dependency on sid-resource-libs. Now, looking at this again, the sid_ucmd_print_exported_kv_store is a bit odd one out there (all the other functions are bound for use in a module somehow). So yes, we should move that functionality somewhere else...

So what if we simply formatted the output on SID (daemon) side and just enhance the protocol to say in what format we need the output. Right now, we dump to internal format on daemon side, then we send the buffer over to sidctl and then sidctl does formatting to a different buffer (so two buffers in play). The new way could save us one extra step/buffer use - the _build_kv_buffer could either build the internal format (used for syncing worker db changes with main db) or the dump format directly (which itself could be in various other formats like JSON, yaml... whatever we need).

Actually, the sidctl tree already works this way (outputs in JSON only right now on daemon side, but that's exactly the protocol enhancement to declare the format we need and then the daemon side formatting it based on that).

[prajnoha]

(On SID daemon side, the formatting would happen in a separate process, working on db snapshot, so we don't need to be afraid that we'd pause the main daemon code from accepting new requests while formatting. And from this perspective, it probably doesn't matter at all whether SID worker does the formatting or the sidctl does, right? But if it's more appropriate for us to do that on SID daemon side, we should follow that path then...)

[bmarzins]

So sure, if things will get simplified, go ahead! I just wanted to present what was meant with the original intention (to hide the difference between BUFFER_MODE_PLAIN and SIZE_PREFIX behing the buffer interface, but we probably can't hide it completely anyway).

So, it turns out I was overlooking some complexity here. Server functions that call buffer_read() currently accept input that comes along with a client side hang up. For _on_connection_event(), this means that after sid gets a command, and responds to the client, it will get a hangup event that sets EPOLLIN and EPOLLHUP. When we try to read the buffer, we will just get the EOF, with no actual data. With the existing code this will return 0, since the buffer has been reset, and is currently empty. So having this special case, where buffer_read() successfully returns 0 for reading no data with an empty buffer, is actually what handles client hangups. Now, we could change size prefixed buffers to always write the prefix, and still handle hangups. But it doesn't really remove complexity like I thought it did, and there's not much point in change for change's sake.

[bmarzins]

Minor cleanups. No functional changes.

[bmarzins]

(On SID daemon side, the formatting would happen in a separate process, working on db snapshot, so we don't need to be afraid that we'd pause the main daemon code from accepting new requests while formatting. And from this perspective, it probably doesn't matter at all whether SID worker does the formatting or the sidctl does, right? But if it's more appropriate for us to do that on SID daemon side, we should follow that path...)

I don't see anything wrong with having SID always responding to commands with a msg header and a series of strings designed to be printable. That's a pretty reasonable format. Things that want easily parsable output can either rely on json formatting, or by getting their output in separate strings, like we do for the udev variables. This does mean that we're transferring more data than we would with a different interface, but since we're currently building an output buffer and then printing it, just transferring the formatted output would take up less total memory than we currently use, for commands that transfer the data via memfds.

[bmarzins]

I'm working changing where we do the formatting, and turning sidctl and usid into wrappers around a library in a separate branch. So from my point of view, this branch in good to merge. If there were any other changes you were waiting for, before merging it, let me know.

[prajnoha]

Sure, let's have the other part separate, will merge this part now...