Libguestfs - May 2022 - [libguestfs PATCH 1/2] guestfs_readdir(): rewrite with FileOut transfer, to lift protocol limit

On 05/01/22 12:47, Richard W.M. Jones wrote:
> 
> On Sun, May 01, 2022 at 09:04:40AM +0200, Laszlo Ersek wrote:
>> Currently the guestfs_readdir() API can not list long directories, due
to
>> it sending back the whole directory listing in a single guestfs
protocol
>> response, which is limited to GUESTFS_MESSAGE_MAX (approx. 4MB) in
size.
>>
>> Introduce the "internal_readdir" action, for transferring the
directory
>> listing from the daemon to the library through a FileOut parameter.
>> Rewrite guestfs_readdir() on top of this new internal function:
>>
>> - The new "internal_readdir" action is a daemon action.
Repurpose the
>>   "readdir" proc_nr (138) for "internal_readdir".
Assume the new action
>>   will first be released in libguestfs-1.48.2.
> 
> So you can't really do this, it's best to allocate a new proc_nr
> (we're not going to run out of integers ...).
> 
> Although the protocol is an internal detail of libguestfs that we can
> change whenever we want, unfortunately some distros ship the binary
> appliance to their users, either the one we supply, or one built
> themselves.
Urgh. I was aware that the proc_nr's must always match, but didn't think
they were treated as public ABI.
> The point being that on those distros you could get a
> mismatch between library & appliance.  Reusing the proc_nr will cause
> weird breakage (probably a hang), whereas using a new number will
> produce a clear error message.
> 
>> - Turn "readdir" from a daemon action into a non-daemon one.
Call the
>>   daemon action guestfs_internal_readdir() manually, receive the
FileOut
>>   parameter into a temp file, then deserialize the dirents array from
the
>>   temp file.
>>
>> This patch sneakily fixes an independent bug, too. In the pre-patch
>> do_readdir() function [daemon/readdir.c], when readdir() returns NULL,
we
>> don't distinguish "end of directory stream" from
"readdir() failed". This
>> rewrite fixes this problem -- I didn't see much value separating
out the
>> fix for the original do_readdir().
>>
>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1674392
>> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
>> ---
>>  lib/Makefile.am           |   1 +
>>  generator/actions_core.ml | 127 ++++++++++---------
>>  generator/proc_nr.ml      |   2 +-
>>  daemon/readdir.c          | 132 ++++++++++----------
>>  lib/readdir.c             | 131 +++++++++++++++++++
>>  TODO                      |   8 --
>>  6 files changed, 266 insertions(+), 135 deletions(-)
> 
> [...]
> 
>> --- a/generator/proc_nr.ml
>> +++ b/generator/proc_nr.ml
>> @@ -152,7 +152,7 @@ let proc_nr = [
>>  135, "mknod_b";
>>  136, "mknod_c";
>>  137, "umask";
>> -138, "readdir";
>> +138, "internal_readdir";
>>  139, "sfdiskM";
>>  140, "zfile";
>>  141, "getxattrs";
> 
> So here, remove the old entry completely and add a new one at the end.
OK!
> 
> [...]
> 
>> +  xdrmem_create (&xdr, xdr_buf, GUESTFS_MAX_CHUNK_SIZE,
XDR_ENCODE);
> 
> I've forgotton how xdrmem works.  Does it realloc the buffer when it
> runs out of space?
No; the encoding that no longer fits fails.

But the patch (and the next one too) make sure we never run out of space
in the xdr buffer. This patch just assumes that a single
guestfs_int_dirent can always be encoded in GUESTFS_MAX_CHUNK_SIZE
(which is a justified assumption). The next one actually enforces it.
> 
> Patch looks generally good to me.
Thanks!
Laszlo
> 
> Rich.
>

On Mon, May 02, 2022 at 08:59:13AM +0200, Laszlo Ersek
wrote:
> On 05/01/22 12:47, Richard W.M. Jones wrote:
> > Although the protocol is an internal detail of libguestfs that we can
> > change whenever we want, unfortunately some distros ship the binary
> > appliance to their users, either the one we supply, or one built
> > themselves.
> 
> Urgh. I was aware that the proc_nr's must always match, but didn't
think
> they were treated as public ABI.
So I want to be clear for the record that they are not public ABI, and
nothing about the protocol is stable.  But nevertheless we shouldn't
break some distros, even if they are doing the wrong thing, when it
only requires a trivial adjustment at our end.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top