[Libguestfs] [libguestfs PATCH v2 2/2] guestfs_readdir(): minimize the number of send_file_write() calls

[Laszlo Ersek]

On 05/03/22 10:42, Richard W.M. Jones wrote:
> On Mon, May 02, 2022 at 10:56:01AM +0200, Laszlo Ersek wrote:
>> In guestfs_readdir(), the daemon currently sends each XDR-encoded
>> "guestfs_int_dirent" to the library with a separate send_file_write()
>> call.
>>
>> Determine the largest encoded size (from the longest filename that a
>> "guestfs_int_dirent" could carry, from readdir()'s "struct dirent"), and
>> batch up the XDR encodings until the next encoding might not fit in
>> GUESTFS_MAX_CHUNK_SIZE. Call send_file_write() only then.
>>
>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1674392
>> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
>> Acked-by: Richard W.M. Jones <rjones at redhat.com>
>> ---
>>
>> Notes:
>>     v2:
>>     
>>     - pick up Rich's ACK
>>
>>  daemon/readdir.c | 38 ++++++++++++++++----
>>  1 file changed, 32 insertions(+), 6 deletions(-)
>>
>> diff --git a/daemon/readdir.c b/daemon/readdir.c
>> index 9ab0b0aec955..3084ba939c10 100644
>> --- a/daemon/readdir.c
>> +++ b/daemon/readdir.c
>> @@ -35,6 +35,9 @@ do_internal_readdir (const char *dir)
>>    DIR *dirstream;
>>    void *xdr_buf;
>>    XDR xdr;
>> +  struct dirent fill;
>> +  guestfs_int_dirent v;
>> +  unsigned max_encoded;
>>  
>>    /* Prepare to fail. */
>>    ret = -1;
>> @@ -55,6 +58,20 @@ do_internal_readdir (const char *dir)
>>    }
>>    xdrmem_create (&xdr, xdr_buf, GUESTFS_MAX_CHUNK_SIZE, XDR_ENCODE);
>>  
>> +  /* Calculate the max number of bytes a "guestfs_int_dirent" can be encoded to.
>> +   */
>> +  memset (fill.d_name, 'a', sizeof fill.d_name - 1);
>> +  fill.d_name[sizeof fill.d_name - 1] = '\0';
>> +  v.ino = INT64_MAX;
>> +  v.ftyp = '?';
>> +  v.name = fill.d_name;
>> +  if (!xdr_guestfs_int_dirent (&xdr, &v)) {
>> +    fprintf (stderr, "xdr_guestfs_int_dirent failed\n");
>> +    goto release_xdr;
>> +  }
>> +  max_encoded = xdr_getpos (&xdr);
>> +  xdr_setpos (&xdr, 0);
>> +
>>    /* Send an "OK" reply, before starting the file transfer. */
>>    reply (NULL, NULL);
>>  
>> @@ -63,7 +80,6 @@ do_internal_readdir (const char *dir)
>>     */
>>    for (;;) {
>>      struct dirent *d;
>> -    guestfs_int_dirent v;
>>  
>>      errno = 0;
>>      d = readdir (dirstream);
>> @@ -94,22 +110,32 @@ do_internal_readdir (const char *dir)
>>      v.ftyp = 'u';
>>  #endif
>>  
>> +    /* Flush "xdr_buf" if we may not have enough room for encoding "v". */
>> +    if (GUESTFS_MAX_CHUNK_SIZE - xdr_getpos (&xdr) < max_encoded) {
>> +      if (send_file_write (xdr_buf, xdr_getpos (&xdr)) != 0)
>> +        break;
>> +
>> +      xdr_setpos (&xdr, 0);
>> +    }
>> +
>>      if (!xdr_guestfs_int_dirent (&xdr, &v)) {
>>        fprintf (stderr, "xdr_guestfs_int_dirent failed\n");
>>        break;
>>      }
>> -
>> -    if (send_file_write (xdr_buf, xdr_getpos (&xdr)) != 0)
>> -      break;
>> -
>> -    xdr_setpos (&xdr, 0);
>>    }
>>  
>> +  /* Flush "xdr_buf" if the loop completed successfully and "xdr_buf" is not
>> +   * empty. */
>> +  if (ret == 0 && xdr_getpos (&xdr) > 0 &&
>> +      send_file_write (xdr_buf, xdr_getpos (&xdr)) != 0)
>> +    ret = -1;
>> +
>>    /* Finish or cancel the transfer. Note that if (ret == -1) because the library
>>     * canceled, we still need to cancel back!
>>     */
>>    send_file_end (ret == -1);
>>  
>> +release_xdr:
>>    xdr_destroy (&xdr);
>>    free (xdr_buf);
> 
> For the whole series:
> 
> Reviewed-by: Richard W.M. Jones <rjones at redhat.com>

Commit range ac00e603f838..4864d21cb8eb.

Thank you!
Laszlo