[Libguestfs] [PATCH v7 11/12] nbd/server: Prepare for per-request filtering of BLOCK_STATUS

[Eric Blake]

The next commit will add support for the optional extension
NBD_CMD_FLAG_PAYLOAD during NBD_CMD_BLOCK_STATUS, where the client can
request that the server only return a subset of negotiated contexts,
rather than all contexts.  To make that task easier, this patch
populates the list of contexts to return on a per-command basis (for
now, identical to the full set of negotiated contexts).

Signed-off-by: Eric Blake 
---

v5: fix null dereference on early error [Vladimir], hoist in assertion
from v4 24/24

v4: split out NBDMetaContexts refactoring to its own patch, track
NBDRequests.contexts as a pointer rather than inline
---
 include/block/nbd.h |  1 +
 nbd/server.c| 22 +-
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 2006497f987..4e7bd6342f9 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -77,6 +77,7 @@ typedef struct NBDRequest {
 uint16_t flags; /* NBD_CMD_FLAG_* */
 uint16_t type;  /* NBD_CMD_* */
 NBDMode mode;   /* Determines which network representation to use */
+NBDMetaContexts *contexts; /* Used by NBD_CMD_BLOCK_STATUS */
 } NBDRequest;

 typedef struct NBDSimpleReply {
diff --git a/nbd/server.c b/nbd/server.c
index 44ebbd139b2..2d4cec75a49 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2505,6 +2505,7 @@ static int coroutine_fn 
nbd_co_receive_request(NBDRequestData *req,
 break;

 case NBD_CMD_BLOCK_STATUS:
+request->contexts = >contexts;
 valid_flags |= NBD_CMD_FLAG_REQ_ONE;
 break;

@@ -2745,17 +2746,18 @@ static coroutine_fn int nbd_handle_request(NBDClient 
*client,
   "discard failed", errp);

 case NBD_CMD_BLOCK_STATUS:
+assert(request->contexts);
 if (!request->len) {
 return nbd_send_generic_reply(client, request, -EINVAL,
   "need non-zero length", errp);
 }
 assert(client->mode >= NBD_MODE_EXTENDED ||
request->len <= UINT32_MAX);
-if (client->contexts.count) {
+if (request->contexts->count) {
 bool dont_fragment = request->flags & NBD_CMD_FLAG_REQ_ONE;
-int contexts_remaining = client->contexts.count;
+int contexts_remaining = request->contexts->count;

-if (client->contexts.base_allocation) {
+if (request->contexts->base_allocation) {
 ret = nbd_co_send_block_status(client, request,
exp->common.blk,
request->from,
@@ -2768,7 +2770,7 @@ static coroutine_fn int nbd_handle_request(NBDClient 
*client,
 }
 }

-if (client->contexts.allocation_depth) {
+if (request->contexts->allocation_depth) {
 ret = nbd_co_send_block_status(client, request,
exp->common.blk,
request->from, request->len,
@@ -2781,8 +2783,9 @@ static coroutine_fn int nbd_handle_request(NBDClient 
*client,
 }
 }

+assert(request->contexts->exp == client->exp);
 for (i = 0; i < client->exp->nr_export_bitmaps; i++) {
-if (!client->contexts.bitmaps[i]) {
+if (!request->contexts->bitmaps[i]) {
 continue;
 }
 ret = nbd_co_send_bitmap(client, request,
@@ -2798,6 +2801,10 @@ static coroutine_fn int nbd_handle_request(NBDClient 
*client,
 assert(!contexts_remaining);

 return 0;
+} else if (client->contexts.count) {
+return nbd_send_generic_reply(client, request, -EINVAL,
+  "CMD_BLOCK_STATUS payload not valid",
+  errp);
 } else {
 return nbd_send_generic_reply(client, request, -EINVAL,
   "CMD_BLOCK_STATUS not negotiated",
@@ -2876,6 +2883,11 @@ static coroutine_fn void nbd_trip(void *opaque)
 } else {
 ret = nbd_handle_request(client, , req->data, _err);
 }
+if (request.contexts && request.contexts != >contexts) {
+assert(request.type == NBD_CMD_BLOCK_STATUS);
+g_free(request.contexts->bitmaps);
+g_free(request.contexts);
+}
 if (ret < 0) {
 error_prepend(_err, "Failed to send reply: ");
 goto disconnect;
-- 
2.41.0

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] [PATCH v7 12/12] nbd/server: Add FLAG_PAYLOAD support to CMD_BLOCK_STATUS

[Eric Blake]

Allow a client to request a subset of negotiated meta contexts.  For
example, a client may ask to use a single connection to learn about
both block status and dirty bitmaps, but where the dirty bitmap
queries only need to be performed on a subset of the disk; forcing the
server to compute that information on block status queries in the rest
of the disk is wasted effort (both at the server, and on the amount of
traffic sent over the wire to be parsed and ignored by the client).

Qemu as an NBD client never requests to use more than one meta
context, so it has no need to use block status payloads.  Testing this
instead requires support from libnbd, which CAN access multiple meta
contexts in parallel from a single NBD connection; an interop test
submitted to the libnbd project at the same time as this patch
demonstrates the feature working, as well as testing some corner cases
(for example, when the payload length is longer than the export
length), although other corner cases (like passing the same id
duplicated) requires a protocol fuzzer because libnbd is not wired up
to break the protocol that badly.

This also includes tweaks to 'qemu-nbd --list' to show when a server
is advertising the capability, and to the testsuite to reflect the
addition to that output.

Of note: qemu will always advertise the new feature bit during
NBD_OPT_INFO if extended headers have alreay been negotiated
(regardless of whether any NBD_OPT_SET_META_CONTEXT negotiation has
occurred); but for NBD_OPT_GO, qemu only advertises the feature if
block status is also enabled (that is, if the client does not
negotiate any contexts, then NBD_CMD_BLOCK_STATUS cannot be used, so
the feature is not advertised).

Signed-off-by: Eric Blake 
---

v5: factor out 'id - NBD_MTA_ID_DIRTY_BITMAP' [Vladimir], rework logic
on zero-length requests to be clearer [Vladimir], rebase to earlier
changes
---
 docs/interop/nbd.txt  |   2 +-
 nbd/server.c  | 114 --
 qemu-nbd.c|   1 +
 nbd/trace-events  |   1 +
 tests/qemu-iotests/223.out|  12 +-
 tests/qemu-iotests/307.out|  10 +-
 .../tests/nbd-qemu-allocation.out |   2 +-
 7 files changed, 122 insertions(+), 20 deletions(-)

diff --git a/docs/interop/nbd.txt b/docs/interop/nbd.txt
index 9aae5e1f294..18efb251de9 100644
--- a/docs/interop/nbd.txt
+++ b/docs/interop/nbd.txt
@@ -69,4 +69,4 @@ NBD_CMD_BLOCK_STATUS for "qemu:dirty-bitmap:", NBD_CMD_CACHE
 NBD_CMD_FLAG_FAST_ZERO
 * 5.2: NBD_CMD_BLOCK_STATUS for "qemu:allocation-depth"
 * 7.1: NBD_FLAG_CAN_MULTI_CONN for shareable writable exports
-* 8.2: NBD_OPT_EXTENDED_HEADERS
+* 8.2: NBD_OPT_EXTENDED_HEADERS, NBD_FLAG_BLOCK_STATUS_PAYLOAD
diff --git a/nbd/server.c b/nbd/server.c
index 2d4cec75a49..898580a9b0b 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -512,6 +512,9 @@ static int nbd_negotiate_handle_export_name(NBDClient 
*client, bool no_zeroes,
 if (client->mode >= NBD_MODE_STRUCTURED) {
 myflags |= NBD_FLAG_SEND_DF;
 }
+if (client->mode >= NBD_MODE_EXTENDED && client->contexts.count) {
+myflags |= NBD_FLAG_BLOCK_STAT_PAYLOAD;
+}
 trace_nbd_negotiate_new_style_size_flags(client->exp->size, myflags);
 stq_be_p(buf, client->exp->size);
 stw_be_p(buf + 8, myflags);
@@ -699,6 +702,10 @@ static int nbd_negotiate_handle_info(NBDClient *client, 
Error **errp)
 if (client->mode >= NBD_MODE_STRUCTURED) {
 myflags |= NBD_FLAG_SEND_DF;
 }
+if (client->mode >= NBD_MODE_EXTENDED &&
+(client->contexts.count || client->opt == NBD_OPT_INFO)) {
+myflags |= NBD_FLAG_BLOCK_STAT_PAYLOAD;
+}
 trace_nbd_negotiate_new_style_size_flags(exp->size, myflags);
 stq_be_p(buf, exp->size);
 stw_be_p(buf + 8, myflags);
@@ -2420,6 +2427,87 @@ static int coroutine_fn nbd_co_send_bitmap(NBDClient 
*client,
 return nbd_co_send_extents(client, request, ea, last, context_id, errp);
 }

+/*
+ * nbd_co_block_status_payload_read
+ * Called when a client wants a subset of negotiated contexts via a
+ * BLOCK_STATUS payload.  Check the payload for valid length and
+ * contents.  On success, return 0 with request updated to effective
+ * length.  If request was invalid but all payload consumed, return 0
+ * with request->len and request->contexts->count set to 0 (which will
+ * trigger an appropriate NBD_EINVAL response later on).  Return
+ * negative errno if the payload was not fully consumed.
+ */
+static int
+nbd_co_block_status_payload_read(NBDClient *client, NBDRequest *request,
+ Error **errp)
+{
+int payload_len = request->len;
+g_autofree char *buf = NULL;
+size_t count, i, nr_bitmaps;
+uint32_t id;
+
+if (payload_len > NBD_MAX_BUFFER_SIZE) {
+error_setg(errp, "len (%" PRIu64 ") is larger than max len (%u)",
+   request->len, 

[Libguestfs] [PATCH v7 08/12] nbd/client: Accept 64-bit block status chunks

[Eric Blake]

Once extended mode is enabled, we need to accept 64-bit status replies
(even for replies that don't exceed a 32-bit length).  It is easier to
normalize narrow replies into wide format so that the rest of our code
only has to handle one width.  Although a server is non-compliant if
it sends a 64-bit reply in compact mode, or a 32-bit reply in extended
mode, it is still easy enough to tolerate these mismatches.

In normal execution, we are only requesting "base:allocation" which
never exceeds 32 bits for flag values. But during testing with
x-dirty-bitmap, we can force qemu to connect to some other context
that might have 64-bit status bit; however, we ignore those upper bits
(other than mapping qemu:allocation-depth into something that
'qemu-img map --output=json' can expose), and since that only affects
testing, we really don't bother with checking whether more than the
two least-significant bits are set.

Signed-off-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---

v5: factor out duplicate length calculation [Vladimir], add R-b

v4: tweak comments and error message about count mismatch, fix setting
of wide in loop [Vladimir]
---
 block/nbd.c| 49 --
 block/trace-events |  1 +
 2 files changed, 35 insertions(+), 15 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 76461430411..52ebc8b2f53 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -615,13 +615,17 @@ static int nbd_parse_offset_hole_payload(BDRVNBDState *s,
  */
 static int nbd_parse_blockstatus_payload(BDRVNBDState *s,
  NBDStructuredReplyChunk *chunk,
- uint8_t *payload, uint64_t 
orig_length,
- NBDExtent32 *extent, Error **errp)
+ uint8_t *payload, bool wide,
+ uint64_t orig_length,
+ NBDExtent64 *extent, Error **errp)
 {
 uint32_t context_id;
+uint32_t count;
+size_t ext_len = wide ? sizeof(*extent) : sizeof(NBDExtent32);
+size_t pay_len = sizeof(context_id) + wide * sizeof(count) + ext_len;

 /* The server succeeded, so it must have sent [at least] one extent */
-if (chunk->length < sizeof(context_id) + sizeof(*extent)) {
+if (chunk->length < pay_len) {
 error_setg(errp, "Protocol error: invalid payload for "
  "NBD_REPLY_TYPE_BLOCK_STATUS");
 return -EINVAL;
@@ -636,8 +640,15 @@ static int nbd_parse_blockstatus_payload(BDRVNBDState *s,
 return -EINVAL;
 }

-extent->length = payload_advance32();
-extent->flags = payload_advance32();
+if (wide) {
+count = payload_advance32();
+extent->length = payload_advance64();
+extent->flags = payload_advance64();
+} else {
+count = 0;
+extent->length = payload_advance32();
+extent->flags = payload_advance32();
+}

 if (extent->length == 0) {
 error_setg(errp, "Protocol error: server sent status chunk with "
@@ -658,7 +669,7 @@ static int nbd_parse_blockstatus_payload(BDRVNBDState *s,
  * (always a safe status, even if it loses information).
  */
 if (s->info.min_block && !QEMU_IS_ALIGNED(extent->length,
-   s->info.min_block)) {
+  s->info.min_block)) {
 trace_nbd_parse_blockstatus_compliance("extent length is unaligned");
 if (extent->length > s->info.min_block) {
 extent->length = QEMU_ALIGN_DOWN(extent->length,
@@ -672,13 +683,15 @@ static int nbd_parse_blockstatus_payload(BDRVNBDState *s,
 /*
  * We used NBD_CMD_FLAG_REQ_ONE, so the server should not have
  * sent us any more than one extent, nor should it have included
- * status beyond our request in that extent. However, it's easy
- * enough to ignore the server's noncompliance without killing the
+ * status beyond our request in that extent. Furthermore, a wide
+ * server should have replied with an accurate count (we left
+ * count at 0 for a narrow server).  However, it's easy enough to
+ * ignore the server's noncompliance without killing the
  * connection; just ignore trailing extents, and clamp things to
  * the length of our request.
  */
-if (chunk->length > sizeof(context_id) + sizeof(*extent)) {
-trace_nbd_parse_blockstatus_compliance("more than one extent");
+if (count != wide || chunk->length > pay_len) {
+trace_nbd_parse_blockstatus_compliance("unexpected extent count");
 }
 if (extent->length > orig_length) {
 extent->length = orig_length;
@@ -1124,7 +1137,7 @@ nbd_co_receive_cmdread_reply(BDRVNBDState *s, uint64_t 
cookie,

 static int coroutine_fn
 nbd_co_receive_blockstatus_reply(BDRVNBDState *s, uint64_t cookie,
- uint64_t 

[Libguestfs] [PATCH v7 10/12] nbd/server: Refactor list of negotiated meta contexts

[Eric Blake]

Peform several minor refactorings of how the list of negotiated meta
contexts is managed, to make upcoming patches easier: Promote the
internal type NBDExportMetaContexts to the public opaque type
NBDMetaContexts, and mark exp const.  Use a shorter member name in
NBDClient.  Hoist calls to nbd_check_meta_context() earlier in their
callers, as the number of negotiated contexts may impact the flags
exposed in regards to an export, which in turn requires a new
parameter.  Drop a redundant parameter to nbd_negotiate_meta_queries.
No semantic change intended on the success path; on the failure path,
dropping context in nbd_check_meta_export even when reporting an error
is safer.

Signed-off-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---

v5: rebase to master, tweak commit message [Vladimir], R-b added

v4: new patch split out from v3 13/14, with smaller impact (quit
trying to separate exp outside of NBDMeataContexts)
---
 include/block/nbd.h |  1 +
 nbd/server.c| 55 -
 2 files changed, 31 insertions(+), 25 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index ba8724f5336..2006497f987 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -29,6 +29,7 @@
 typedef struct NBDExport NBDExport;
 typedef struct NBDClient NBDClient;
 typedef struct NBDClientConnection NBDClientConnection;
+typedef struct NBDMetaContexts NBDMetaContexts;

 extern const BlockExportDriver blk_exp_nbd;

diff --git a/nbd/server.c b/nbd/server.c
index b09ee44c159..44ebbd139b2 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -105,11 +105,13 @@ struct NBDExport {

 static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);

-/* NBDExportMetaContexts represents a list of contexts to be exported,
+/*
+ * NBDMetaContexts represents a list of meta contexts in use,
  * as selected by NBD_OPT_SET_META_CONTEXT. Also used for
- * NBD_OPT_LIST_META_CONTEXT. */
-typedef struct NBDExportMetaContexts {
-NBDExport *exp;
+ * NBD_OPT_LIST_META_CONTEXT.
+ */
+struct NBDMetaContexts {
+const NBDExport *exp; /* associated export */
 size_t count; /* number of negotiated contexts */
 bool base_allocation; /* export base:allocation context (block status) */
 bool allocation_depth; /* export qemu:allocation-depth */
@@ -117,7 +119,7 @@ typedef struct NBDExportMetaContexts {
 * export qemu:dirty-bitmap:,
 * sized by exp->nr_export_bitmaps
 */
-} NBDExportMetaContexts;
+};

 struct NBDClient {
 int refcount;
@@ -144,7 +146,7 @@ struct NBDClient {
 uint32_t check_align; /* If non-zero, check for aligned client requests */

 NBDMode mode;
-NBDExportMetaContexts export_meta;
+NBDMetaContexts contexts; /* Negotiated meta contexts */

 uint32_t opt; /* Current option being negotiated */
 uint32_t optlen; /* remaining length of data in ioc for the option being
@@ -455,10 +457,10 @@ static int nbd_negotiate_handle_list(NBDClient *client, 
Error **errp)
 return nbd_negotiate_send_rep(client, NBD_REP_ACK, errp);
 }

-static void nbd_check_meta_export(NBDClient *client)
+static void nbd_check_meta_export(NBDClient *client, NBDExport *exp)
 {
-if (client->exp != client->export_meta.exp) {
-client->export_meta.count = 0;
+if (exp != client->contexts.exp) {
+client->contexts.count = 0;
 }
 }

@@ -504,6 +506,7 @@ static int nbd_negotiate_handle_export_name(NBDClient 
*client, bool no_zeroes,
 error_setg(errp, "export not found");
 return -EINVAL;
 }
+nbd_check_meta_export(client, client->exp);

 myflags = client->exp->nbdflags;
 if (client->mode >= NBD_MODE_STRUCTURED) {
@@ -521,7 +524,6 @@ static int nbd_negotiate_handle_export_name(NBDClient 
*client, bool no_zeroes,

 QTAILQ_INSERT_TAIL(>exp->clients, client, next);
 blk_exp_ref(>exp->common);
-nbd_check_meta_export(client);

 return 0;
 }
@@ -641,6 +643,9 @@ static int nbd_negotiate_handle_info(NBDClient *client, 
Error **errp)
   errp, "export '%s' not present",
   sane_name);
 }
+if (client->opt == NBD_OPT_GO) {
+nbd_check_meta_export(client, exp);
+}

 /* Don't bother sending NBD_INFO_NAME unless client requested it */
 if (sendname) {
@@ -729,7 +734,6 @@ static int nbd_negotiate_handle_info(NBDClient *client, 
Error **errp)
 client->check_align = check_align;
 QTAILQ_INSERT_TAIL(>exp->clients, client, next);
 blk_exp_ref(>exp->common);
-nbd_check_meta_export(client);
 rc = 1;
 }
 return rc;
@@ -852,7 +856,7 @@ static bool nbd_strshift(const char **str, const char 
*prefix)
  * Handle queries to 'base' namespace. For now, only the base:allocation
  * context is available.  Return true if @query has been handled.
  */
-static bool nbd_meta_base_query(NBDClient *client, 

[Libguestfs] [PATCH v7 09/12] nbd/client: Request extended headers during negotiation

[Eric Blake]

All the pieces are in place for a client to finally request extended
headers.  Note that we must not request extended headers when qemu-nbd
is used to connect to the kernel module (as nbd.ko does not expect
them, but expects us to do the negotiation in userspace before handing
the socket over to the kernel), but there is no harm in all other
clients requesting them.

Extended headers are not essential to the information collected during
'qemu-nbd --list', but probing for it gives us one more piece of
information in that output.  Update the iotests affected by the new
line of output.

Signed-off-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---

v5: add R-b

v4: rebase to earlier changes, tweak commit message for why qemu-nbd
connection to /dev/nbd cannot use extended mode [Vladimir]
---
 nbd/client-connection.c   |  2 +-
 nbd/client.c  | 20 ++-
 qemu-nbd.c|  3 +++
 tests/qemu-iotests/223.out|  6 ++
 tests/qemu-iotests/233.out|  4 
 tests/qemu-iotests/241.out|  3 +++
 tests/qemu-iotests/307.out|  5 +
 .../tests/nbd-qemu-allocation.out |  1 +
 8 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/nbd/client-connection.c b/nbd/client-connection.c
index aa0201b7107..f9da67c87e3 100644
--- a/nbd/client-connection.c
+++ b/nbd/client-connection.c
@@ -93,7 +93,7 @@ NBDClientConnection *nbd_client_connection_new(const 
SocketAddress *saddr,
 .do_negotiation = do_negotiation,

 .initial_info.request_sizes = true,
-.initial_info.mode = NBD_MODE_STRUCTURED,
+.initial_info.mode = NBD_MODE_EXTENDED,
 .initial_info.base_allocation = true,
 .initial_info.x_dirty_bitmap = g_strdup(x_dirty_bitmap),
 .initial_info.name = g_strdup(export_name ?: "")
diff --git a/nbd/client.c b/nbd/client.c
index a2f253062aa..29ffc609a4b 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -953,15 +953,23 @@ static int nbd_start_negotiate(QIOChannel *ioc, 
QCryptoTLSCreds *tlscreds,
 if (fixedNewStyle) {
 int result = 0;

+if (max_mode >= NBD_MODE_EXTENDED) {
+result = nbd_request_simple_option(ioc,
+   NBD_OPT_EXTENDED_HEADERS,
+   false, errp);
+if (result) {
+return result < 0 ? -EINVAL : NBD_MODE_EXTENDED;
+}
+}
 if (max_mode >= NBD_MODE_STRUCTURED) {
 result = nbd_request_simple_option(ioc,
NBD_OPT_STRUCTURED_REPLY,
false, errp);
-if (result < 0) {
-return -EINVAL;
+if (result) {
+return result < 0 ? -EINVAL : NBD_MODE_STRUCTURED;
 }
 }
-return result ? NBD_MODE_STRUCTURED : NBD_MODE_SIMPLE;
+return NBD_MODE_SIMPLE;
 } else {
 return NBD_MODE_EXPORT_NAME;
 }
@@ -1034,6 +1042,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, 
QCryptoTLSCreds *tlscreds,
 }

 switch (info->mode) {
+case NBD_MODE_EXTENDED:
 case NBD_MODE_STRUCTURED:
 if (base_allocation) {
 result = nbd_negotiate_simple_meta_context(ioc, info, errp);
@@ -1144,7 +1153,7 @@ int nbd_receive_export_list(QIOChannel *ioc, 
QCryptoTLSCreds *tlscreds,

 *info = NULL;
 result = nbd_start_negotiate(ioc, tlscreds, hostname, ,
- NBD_MODE_STRUCTURED, NULL, errp);
+ NBD_MODE_EXTENDED, NULL, errp);
 if (tlscreds && sioc) {
 ioc = sioc;
 }
@@ -1155,6 +1164,7 @@ int nbd_receive_export_list(QIOChannel *ioc, 
QCryptoTLSCreds *tlscreds,
 switch ((NBDMode)result) {
 case NBD_MODE_SIMPLE:
 case NBD_MODE_STRUCTURED:
+case NBD_MODE_EXTENDED:
 /* newstyle - use NBD_OPT_LIST to populate array, then try
  * NBD_OPT_INFO on each array member. If structured replies
  * are enabled, also try NBD_OPT_LIST_META_CONTEXT. */
@@ -1191,7 +1201,7 @@ int nbd_receive_export_list(QIOChannel *ioc, 
QCryptoTLSCreds *tlscreds,
 break;
 }

-if (result == NBD_MODE_STRUCTURED &&
+if (result >= NBD_MODE_STRUCTURED &&
 nbd_list_meta_contexts(ioc, [i], errp) < 0) {
 goto out;
 }
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 70aa3c487a0..e6681450cfe 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -235,6 +235,9 @@ static int qemu_nbd_client_list(SocketAddress *saddr, 
QCryptoTLSCreds *tls,
 printf("  opt block: %u\n", list[i].opt_block);
 printf("  max block: %u\n", list[i].max_block);
 }

[Libguestfs] [PATCH v7 07/12] nbd/client: Initial support for extended headers

[Eric Blake]

Update the client code to be able to send an extended request, and
parse an extended header from the server.  Note that since we reject
any structured reply with a too-large payload, we can always normalize
a valid header back into the compact form, so that the caller need not
deal with two branches of a union.  Still, until a later patch lets
the client negotiate extended headers, the code added here should not
be reached.  Note that because of the different magic numbers, it is
just as easy to trace and then tolerate a non-compliant server sending
the wrong header reply as it would be to insist that the server is
compliant.

Signed-off-by: Eric Blake 
---

v5: fix logic bug on error reporting [Vladimir]

v4: split off errp handling to separate patch [Vladimir], better
function naming [Vladimir]
---
 include/block/nbd.h |   3 +-
 block/nbd.c |   2 +-
 nbd/client.c| 104 +---
 nbd/trace-events|   3 +-
 4 files changed, 74 insertions(+), 38 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 8a765e78dfb..ba8724f5336 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -389,7 +389,8 @@ int nbd_init(int fd, QIOChannelSocket *sioc, NBDExportInfo 
*info,
  Error **errp);
 int nbd_send_request(QIOChannel *ioc, NBDRequest *request);
 int coroutine_fn nbd_receive_reply(BlockDriverState *bs, QIOChannel *ioc,
-   NBDReply *reply, Error **errp);
+   NBDReply *reply, NBDMode mode,
+   Error **errp);
 int nbd_client(int fd);
 int nbd_disconnect(int fd);
 int nbd_errno_to_system_errno(int err);
diff --git a/block/nbd.c b/block/nbd.c
index 22d3cb11ac8..76461430411 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -458,7 +458,7 @@ static coroutine_fn int nbd_receive_replies(BDRVNBDState 
*s, uint64_t cookie,

 /* We are under mutex and cookie is 0. We have to do the dirty work. */
 assert(s->reply.cookie == 0);
-ret = nbd_receive_reply(s->bs, s->ioc, >reply, errp);
+ret = nbd_receive_reply(s->bs, s->ioc, >reply, s->info.mode, errp);
 if (ret == 0) {
 ret = -EIO;
 error_setg(errp, "server dropped connection");
diff --git a/nbd/client.c b/nbd/client.c
index cecb0f04377..a2f253062aa 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -1346,22 +1346,29 @@ int nbd_disconnect(int fd)

 int nbd_send_request(QIOChannel *ioc, NBDRequest *request)
 {
-uint8_t buf[NBD_REQUEST_SIZE];
+uint8_t buf[NBD_EXTENDED_REQUEST_SIZE];
+size_t len;

-assert(request->mode <= NBD_MODE_STRUCTURED); /* TODO handle extended */
-assert(request->len <= UINT32_MAX);
 trace_nbd_send_request(request->from, request->len, request->cookie,
request->flags, request->type,
nbd_cmd_lookup(request->type));

-stl_be_p(buf, NBD_REQUEST_MAGIC);
 stw_be_p(buf + 4, request->flags);
 stw_be_p(buf + 6, request->type);
 stq_be_p(buf + 8, request->cookie);
 stq_be_p(buf + 16, request->from);
-stl_be_p(buf + 24, request->len);
+if (request->mode >= NBD_MODE_EXTENDED) {
+stl_be_p(buf, NBD_EXTENDED_REQUEST_MAGIC);
+stq_be_p(buf + 24, request->len);
+len = NBD_EXTENDED_REQUEST_SIZE;
+} else {
+assert(request->len <= UINT32_MAX);
+stl_be_p(buf, NBD_REQUEST_MAGIC);
+stl_be_p(buf + 24, request->len);
+len = NBD_REQUEST_SIZE;
+}

-return nbd_write(ioc, buf, sizeof(buf), NULL);
+return nbd_write(ioc, buf, len, NULL);
 }

 /* nbd_receive_simple_reply
@@ -1388,30 +1395,36 @@ static int nbd_receive_simple_reply(QIOChannel *ioc, 
NBDSimpleReply *reply,
 return 0;
 }

-/* nbd_receive_structured_reply_chunk
+/* nbd_receive_reply_chunk_header
  * Read structured reply chunk except magic field (which should be already
- * read).
+ * read).  Normalize into the compact form.
  * Payload is not read.
  */
-static int nbd_receive_structured_reply_chunk(QIOChannel *ioc,
-  NBDStructuredReplyChunk *chunk,
-  Error **errp)
+static int nbd_receive_reply_chunk_header(QIOChannel *ioc, NBDReply *chunk,
+  Error **errp)
 {
 int ret;
+size_t len;
+uint64_t payload_len;

-assert(chunk->magic == NBD_STRUCTURED_REPLY_MAGIC);
+if (chunk->magic == NBD_STRUCTURED_REPLY_MAGIC) {
+len = sizeof(chunk->structured);
+} else {
+assert(chunk->magic == NBD_EXTENDED_REPLY_MAGIC);
+len = sizeof(chunk->extended);
+}

 ret = nbd_read(ioc, (uint8_t *)chunk + sizeof(chunk->magic),
-   sizeof(*chunk) - sizeof(chunk->magic), "structured chunk",
+   len - sizeof(chunk->magic), "structured chunk",
errp);
 if (ret < 0) {
 return ret;
 }

-

[Libguestfs] [PATCH v7 05/12] nbd/server: Enable initial support for extended headers

[Eric Blake]

Time to start supporting clients that request extended headers.  Now
we can finally reach the code added across several previous patches.

Even though the NBD spec has been altered to allow us to accept
NBD_CMD_READ larger than the max payload size (provided our response
is a hole or broken up over more than one data chunk), we are not
planning to take advantage of that, and continue to cap NBD_CMD_READ
to 32M regardless of header size.

For NBD_CMD_WRITE_ZEROES and NBD_CMD_TRIM, the block layer already
supports 64-bit operations without any effort on our part.  For
NBD_CMD_BLOCK_STATUS, the client's length is a hint, and the previous
patch took care of implementing the required
NBD_REPLY_TYPE_BLOCK_STATUS_EXT.

We do not yet support clients that want to do request payload
filtering of NBD_CMD_BLOCK_STATUS; that will be added in later
patches, but is not essential for qemu as a client since qemu only
requests the single context base:allocation.

Signed-off-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---

v5: add R-b, s/8.1/8.2/

v4: split out parts into earlier patches, rebase to earlier changes,
simplify handling of generic replies, retitle (compare to v3 9/14)
---
 docs/interop/nbd.txt |  1 +
 nbd/server.c | 21 +
 2 files changed, 22 insertions(+)

diff --git a/docs/interop/nbd.txt b/docs/interop/nbd.txt
index f5ca25174a6..9aae5e1f294 100644
--- a/docs/interop/nbd.txt
+++ b/docs/interop/nbd.txt
@@ -69,3 +69,4 @@ NBD_CMD_BLOCK_STATUS for "qemu:dirty-bitmap:", NBD_CMD_CACHE
 NBD_CMD_FLAG_FAST_ZERO
 * 5.2: NBD_CMD_BLOCK_STATUS for "qemu:allocation-depth"
 * 7.1: NBD_FLAG_CAN_MULTI_CONN for shareable writable exports
+* 8.2: NBD_OPT_EXTENDED_HEADERS
diff --git a/nbd/server.c b/nbd/server.c
index 8448167b12a..b09ee44c159 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -482,6 +482,10 @@ static int nbd_negotiate_handle_export_name(NBDClient 
*client, bool no_zeroes,
 [10 .. 133]   reserved (0) [unless no_zeroes]
  */
 trace_nbd_negotiate_handle_export_name();
+if (client->mode >= NBD_MODE_EXTENDED) {
+error_setg(errp, "Extended headers already negotiated");
+return -EINVAL;
+}
 if (client->optlen > NBD_MAX_STRING_SIZE) {
 error_setg(errp, "Bad length received");
 return -EINVAL;
@@ -1264,6 +1268,10 @@ static int nbd_negotiate_options(NBDClient *client, 
Error **errp)
 case NBD_OPT_STRUCTURED_REPLY:
 if (length) {
 ret = nbd_reject_length(client, false, errp);
+} else if (client->mode >= NBD_MODE_EXTENDED) {
+ret = nbd_negotiate_send_rep_err(
+client, NBD_REP_ERR_EXT_HEADER_REQD, errp,
+"extended headers already negotiated");
 } else if (client->mode >= NBD_MODE_STRUCTURED) {
 ret = nbd_negotiate_send_rep_err(
 client, NBD_REP_ERR_INVALID, errp,
@@ -1280,6 +1288,19 @@ static int nbd_negotiate_options(NBDClient *client, 
Error **errp)
  errp);
 break;

+case NBD_OPT_EXTENDED_HEADERS:
+if (length) {
+ret = nbd_reject_length(client, false, errp);
+} else if (client->mode >= NBD_MODE_EXTENDED) {
+ret = nbd_negotiate_send_rep_err(
+client, NBD_REP_ERR_INVALID, errp,
+"extended headers already negotiated");
+} else {
+ret = nbd_negotiate_send_rep(client, NBD_REP_ACK, errp);
+client->mode = NBD_MODE_EXTENDED;
+}
+break;
+
 default:
 ret = nbd_opt_drop(client, NBD_REP_ERR_UNSUP, errp,
"Unsupported option %" PRIu32 " (%s)",
-- 
2.41.0

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] [PATCH v7 04/12] nbd/server: Support 64-bit block status

[Eric Blake]

The NBD spec states that if the client negotiates extended headers,
the server must avoid NBD_REPLY_TYPE_BLOCK_STATUS and instead use
NBD_REPLY_TYPE_BLOCK_STATUS_EXT which supports 64-bit lengths, even if
the reply does not need more than 32 bits.  As of this patch,
client->mode is still never NBD_MODE_EXTENDED, so the code added here
does not take effect until the next patch enables negotiation.

For now, all metacontexts that we know how to export never populate
more than 32 bits of information, so we don't have to worry about
NBD_REP_ERR_EXT_HEADER_REQD or filtering during handshake, and we
always send all zeroes for the upper 32 bits of status during
NBD_CMD_BLOCK_STATUS.

Note that we previously had some interesting size-juggling on call
chains, such as:

nbd_co_send_block_status(uint32_t length)
-> blockstatus_to_extents(uint32_t bytes)
  -> bdrv_block_status_above(bytes, _t num)
  -> nbd_extent_array_add(uint64_t num)
-> store num in 32-bit length

But we were lucky that it never overflowed: bdrv_block_status_above
never sets num larger than bytes, and we had previously been capping
'bytes' at 32 bits (since the protocol does not allow sending a larger
request without extended headers).  This patch adds some assertions
that ensure we continue to avoid overflowing 32 bits for a narrow
client, while fully utilizing 64-bits all the way through when the
client understands that.  Even in 64-bit math, overflow is not an
issue, because all lengths are coming from the block layer, and we
know that the block layer does not support images larger than off_t
(if lengths were coming from the network, the story would be
different).

Signed-off-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---

v5: stronger justification on assertion [Vladimir], add R-b

v4: split conversion to big-endian across two helper functions rather
than in-place union [Vladimir]
---
 nbd/server.c | 108 ++-
 1 file changed, 82 insertions(+), 26 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 4cd061f9da4..8448167b12a 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2103,20 +2103,24 @@ static int coroutine_fn 
nbd_co_send_sparse_read(NBDClient *client,
 }

 typedef struct NBDExtentArray {
-NBDExtent32 *extents;
+NBDExtent64 *extents;
 unsigned int nb_alloc;
 unsigned int count;
 uint64_t total_length;
+bool extended;
 bool can_add;
 bool converted_to_be;
 } NBDExtentArray;

-static NBDExtentArray *nbd_extent_array_new(unsigned int nb_alloc)
+static NBDExtentArray *nbd_extent_array_new(unsigned int nb_alloc,
+NBDMode mode)
 {
 NBDExtentArray *ea = g_new0(NBDExtentArray, 1);

+assert(mode >= NBD_MODE_STRUCTURED);
 ea->nb_alloc = nb_alloc;
-ea->extents = g_new(NBDExtent32, nb_alloc);
+ea->extents = g_new(NBDExtent64, nb_alloc);
+ea->extended = mode >= NBD_MODE_EXTENDED;
 ea->can_add = true;

 return ea;
@@ -2135,15 +2139,36 @@ static void 
nbd_extent_array_convert_to_be(NBDExtentArray *ea)
 int i;

 assert(!ea->converted_to_be);
+assert(ea->extended);
 ea->can_add = false;
 ea->converted_to_be = true;

 for (i = 0; i < ea->count; i++) {
-ea->extents[i].flags = cpu_to_be32(ea->extents[i].flags);
-ea->extents[i].length = cpu_to_be32(ea->extents[i].length);
+ea->extents[i].length = cpu_to_be64(ea->extents[i].length);
+ea->extents[i].flags = cpu_to_be64(ea->extents[i].flags);
 }
 }

+/* Further modifications of the array after conversion are abandoned */
+static NBDExtent32 *nbd_extent_array_convert_to_narrow(NBDExtentArray *ea)
+{
+int i;
+NBDExtent32 *extents = g_new(NBDExtent32, ea->count);
+
+assert(!ea->converted_to_be);
+assert(!ea->extended);
+ea->can_add = false;
+ea->converted_to_be = true;
+
+for (i = 0; i < ea->count; i++) {
+assert((ea->extents[i].length | ea->extents[i].flags) <= UINT32_MAX);
+extents[i].length = cpu_to_be32(ea->extents[i].length);
+extents[i].flags = cpu_to_be32(ea->extents[i].flags);
+}
+
+return extents;
+}
+
 /*
  * Add extent to NBDExtentArray. If extent can't be added (no available space),
  * return -1.
@@ -2154,19 +2179,27 @@ static void 
nbd_extent_array_convert_to_be(NBDExtentArray *ea)
  * would result in an incorrect range reported to the client)
  */
 static int nbd_extent_array_add(NBDExtentArray *ea,
-uint32_t length, uint32_t flags)
+uint64_t length, uint32_t flags)
 {
 assert(ea->can_add);

 if (!length) {
 return 0;
 }
+if (!ea->extended) {
+assert(length <= UINT32_MAX);
+}

 /* Extend previous extent if flags are the same */
 if (ea->count > 0 && flags == ea->extents[ea->count - 1].flags) {
-uint64_t sum = (uint64_t)length + ea->extents[ea->count - 1].length;
+uint64_t sum = 

[Libguestfs] [PATCH v7 03/12] nbd/server: Prepare to send extended header replies

[Eric Blake]

Although extended mode is not yet enabled, once we do turn it on, we
need to reply with extended headers to all messages.  Update the low
level entry points necessary so that all other callers automatically
get the right header based on the current mode.

Signed-off-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---

v5: s/iov->iov_len/iov[0].iov_len/ [Vladimir], add R-b

v4: new patch, split out from v3 9/14
---
 nbd/server.c | 30 ++
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index e227e470d41..4cd061f9da4 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1938,8 +1938,6 @@ static inline void set_be_chunk(NBDClient *client, struct 
iovec *iov,
 size_t niov, uint16_t flags, uint16_t type,
 NBDRequest *request)
 {
-/* TODO - handle structured vs. extended replies */
-NBDStructuredReplyChunk *chunk = iov->iov_base;
 size_t i, length = 0;

 for (i = 1; i < niov; i++) {
@@ -1947,12 +1945,26 @@ static inline void set_be_chunk(NBDClient *client, 
struct iovec *iov,
 }
 assert(length <= NBD_MAX_BUFFER_SIZE + sizeof(NBDStructuredReadData));

-iov[0].iov_len = sizeof(*chunk);
-stl_be_p(>magic, NBD_STRUCTURED_REPLY_MAGIC);
-stw_be_p(>flags, flags);
-stw_be_p(>type, type);
-stq_be_p(>cookie, request->cookie);
-stl_be_p(>length, length);
+if (client->mode >= NBD_MODE_EXTENDED) {
+NBDExtendedReplyChunk *chunk = iov->iov_base;
+
+iov[0].iov_len = sizeof(*chunk);
+stl_be_p(>magic, NBD_EXTENDED_REPLY_MAGIC);
+stw_be_p(>flags, flags);
+stw_be_p(>type, type);
+stq_be_p(>cookie, request->cookie);
+stq_be_p(>offset, request->from);
+stq_be_p(>length, length);
+} else {
+NBDStructuredReplyChunk *chunk = iov->iov_base;
+
+iov[0].iov_len = sizeof(*chunk);
+stl_be_p(>magic, NBD_STRUCTURED_REPLY_MAGIC);
+stw_be_p(>flags, flags);
+stw_be_p(>type, type);
+stq_be_p(>cookie, request->cookie);
+stl_be_p(>length, length);
+}
 }

 static int coroutine_fn nbd_co_send_chunk_done(NBDClient *client,
@@ -2509,6 +2521,8 @@ static coroutine_fn int nbd_send_generic_reply(NBDClient 
*client,
 {
 if (client->mode >= NBD_MODE_STRUCTURED && ret < 0) {
 return nbd_co_send_chunk_error(client, request, -ret, error_msg, errp);
+} else if (client->mode >= NBD_MODE_EXTENDED) {
+return nbd_co_send_chunk_done(client, request, errp);
 } else {
 return nbd_co_send_simple_reply(client, request, ret < 0 ? -ret : 0,
 NULL, 0, errp);
-- 
2.41.0

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] [PATCH v7 06/12] nbd/client: Plumb errp through nbd_receive_replies

[Eric Blake]

Instead of ignoring the low-level error just to refabricate our own
message to pass to the caller, we can just plumb the caller's errp
down to the low level.

Signed-off-by: Eric Blake 
---

v5: set errp on more failure cases [Vladimir], typo fix

v4: new patch [Vladimir]
---
 block/nbd.c | 18 --
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 4a7f37da1c6..22d3cb11ac8 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -416,7 +416,8 @@ static void coroutine_fn GRAPH_RDLOCK 
nbd_reconnect_attempt(BDRVNBDState *s)
 reconnect_delay_timer_del(s);
 }

-static coroutine_fn int nbd_receive_replies(BDRVNBDState *s, uint64_t cookie)
+static coroutine_fn int nbd_receive_replies(BDRVNBDState *s, uint64_t cookie,
+Error **errp)
 {
 int ret;
 uint64_t ind = COOKIE_TO_INDEX(cookie), ind2;
@@ -457,20 +458,25 @@ static coroutine_fn int nbd_receive_replies(BDRVNBDState 
*s, uint64_t cookie)

 /* We are under mutex and cookie is 0. We have to do the dirty work. */
 assert(s->reply.cookie == 0);
-ret = nbd_receive_reply(s->bs, s->ioc, >reply, NULL);
-if (ret <= 0) {
-ret = ret ? ret : -EIO;
+ret = nbd_receive_reply(s->bs, s->ioc, >reply, errp);
+if (ret == 0) {
+ret = -EIO;
+error_setg(errp, "server dropped connection");
+}
+if (ret < 0) {
 nbd_channel_error(s, ret);
 return ret;
 }
 if (nbd_reply_is_structured(>reply) &&
 s->info.mode < NBD_MODE_STRUCTURED) {
 nbd_channel_error(s, -EINVAL);
+error_setg(errp, "unexpected structured reply");
 return -EINVAL;
 }
 ind2 = COOKIE_TO_INDEX(s->reply.cookie);
 if (ind2 >= MAX_NBD_REQUESTS || !s->requests[ind2].coroutine) {
 nbd_channel_error(s, -EINVAL);
+error_setg(errp, "unexpected cookie value");
 return -EINVAL;
 }
 if (s->reply.cookie == cookie) {
@@ -842,9 +848,9 @@ static coroutine_fn int nbd_co_do_receive_one_chunk(
 }
 *request_ret = 0;

-ret = nbd_receive_replies(s, cookie);
+ret = nbd_receive_replies(s, cookie, errp);
 if (ret < 0) {
-error_setg(errp, "Connection closed");
+error_prepend(errp, "Connection closed: ");
 return -EIO;
 }
 assert(s->ioc);
-- 
2.41.0

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] [PATCH v7 01/12] nbd/server: Support a request payload

[Eric Blake]

Upcoming additions to support NBD 64-bit effect lengths allow for the
possibility to distinguish between payload length (capped at 32M) and
effect length (64 bits, although we generally assume 63 bits because
of off_t limitations).  Without that extension, only the NBD_CMD_WRITE
request has a payload; but with the extension, it makes sense to allow
at least NBD_CMD_BLOCK_STATUS to have both a payload and effect length
in a future patch (where the payload is a limited-size struct that in
turn gives the real effect length as well as a subset of known ids for
which status is requested).  Other future NBD commands may also have a
request payload, so the 64-bit extension introduces a new
NBD_CMD_FLAG_PAYLOAD_LEN that distinguishes between whether the header
length is a payload length or an effect length, rather than
hard-coding the decision based on the command.

According to the spec, a client should never send a command with a
payload without the negotiation phase proving such extension is
available.  So in the unlikely event the bit is set or cleared
incorrectly, the client is already at fault; if the client then
provides the payload, we can gracefully consume it off the wire and
fail the command with NBD_EINVAL (subsequent checks for magic numbers
ensure we are still in sync), while if the client fails to send
payload we block waiting for it (basically deadlocking our connection
to the bad client, but not negatively impacting our ability to service
other clients, so not a security risk).  Note that we do not support
the payload version of BLOCK_STATUS yet.

Signed-off-by: Eric Blake 
---

v7: another try at better logic [Vladimir]

v5: retitled from v4 13/24, rewrite on top of previous patch's switch
statement [Vladimir]

v4: less indentation on several 'if's [Vladimir]
---
 nbd/server.c | 37 +
 nbd/trace-events |  1 +
 2 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 7a6f95071f8..1eabcfc908d 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2322,9 +2322,11 @@ static int coroutine_fn 
nbd_co_receive_request(NBDRequestData *req,
Error **errp)
 {
 NBDClient *client = req->client;
+bool extended_with_payload;
 bool check_length = false;
 bool check_rofs = false;
 bool allocate_buffer = false;
+bool payload_okay = false;
 unsigned payload_len = 0;
 int valid_flags = NBD_CMD_FLAG_FUA;
 int ret;
@@ -2338,6 +2340,13 @@ static int coroutine_fn 
nbd_co_receive_request(NBDRequestData *req,

 trace_nbd_co_receive_request_decode_type(request->cookie, request->type,
  nbd_cmd_lookup(request->type));
+extended_with_payload = client->mode >= NBD_MODE_EXTENDED &&
+request->flags & NBD_CMD_FLAG_PAYLOAD_LEN;
+if (extended_with_payload) {
+payload_len = request->len;
+check_length = true;
+}
+
 switch (request->type) {
 case NBD_CMD_DISC:
 /* Special case: we're going to disconnect without a reply,
@@ -2354,6 +2363,15 @@ static int coroutine_fn 
nbd_co_receive_request(NBDRequestData *req,
 break;

 case NBD_CMD_WRITE:
+if (client->mode >= NBD_MODE_EXTENDED) {
+if (!extended_with_payload) {
+/* The client is noncompliant. Trace it, but proceed. */
+trace_nbd_co_receive_ext_payload_compliance(request->from,
+request->len);
+}
+valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN;
+}
+payload_okay = true;
 payload_len = request->len;
 check_length = true;
 allocate_buffer = true;
@@ -2395,6 +2413,14 @@ static int coroutine_fn 
nbd_co_receive_request(NBDRequestData *req,
request->len, NBD_MAX_BUFFER_SIZE);
 return -EINVAL;
 }
+if (payload_len && !payload_okay) {
+/*
+ * For now, we don't support payloads on other commands; but
+ * we can keep the connection alive by ignoring the payload.
+ */
+assert(request->type != NBD_CMD_WRITE);
+request->len = 0;
+}
 if (allocate_buffer) {
 /* READ, WRITE */
 req->data = blk_try_blockalign(client->exp->common.blk,
@@ -2405,10 +2431,13 @@ static int coroutine_fn 
nbd_co_receive_request(NBDRequestData *req,
 }
 }
 if (payload_len) {
-/* WRITE */
-assert(req->data);
-ret = nbd_read(client->ioc, req->data, payload_len,
-   "CMD_WRITE data", errp);
+if (payload_okay) {
+assert(req->data);
+ret = nbd_read(client->ioc, req->data, payload_len,
+   "CMD_WRITE data", errp);
+} else {
+ret = nbd_drop(client->ioc, payload_len, errp);
+}
 if (ret < 0) {
 return -EIO;
 }
diff --git 

[Libguestfs] [PATCH v7 02/12] nbd/server: Prepare to receive extended header requests

[Eric Blake]

Although extended mode is not yet enabled, once we do turn it on, we
need to accept extended requests for all messages.  Previous patches
have already taken care of supporting 64-bit lengths, now we just need
to read it off the wire.

Note that this implementation will block indefinitely on a buggy
client that sends a non-extended payload (that is, we try to read a
full packet before we ever check the magic number, but a client that
mistakenly sends a simple request after negotiating extended headers
doesn't send us enough bytes), but it's no different from any other
client that stops talking to us partway through a packet and thus not
worth coding around.

Signed-off-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---

v6: fix sign extension bug

v5: no change

v4: new patch, split out from v3 9/14
---
 nbd/nbd-internal.h |  5 -
 nbd/server.c   | 43 ++-
 2 files changed, 34 insertions(+), 14 deletions(-)

diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index 133b1d94b50..dfa02f77ee4 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -34,8 +34,11 @@
  * https://github.com/yoe/nbd/blob/master/doc/proto.md
  */

-/* Size of all NBD_OPT_*, without payload */
+/* Size of all compact NBD_CMD_*, without payload */
 #define NBD_REQUEST_SIZE(4 + 2 + 2 + 8 + 8 + 4)
+/* Size of all extended NBD_CMD_*, without payload */
+#define NBD_EXTENDED_REQUEST_SIZE   (4 + 2 + 2 + 8 + 8 + 8)
+
 /* Size of all NBD_REP_* sent in answer to most NBD_OPT_*, without payload */
 #define NBD_REPLY_SIZE  (4 + 4 + 8)
 /* Size of reply to NBD_OPT_EXPORT_NAME */
diff --git a/nbd/server.c b/nbd/server.c
index 1eabcfc908d..e227e470d41 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1411,11 +1411,13 @@ nbd_read_eof(NBDClient *client, void *buffer, size_t 
size, Error **errp)
 static int coroutine_fn nbd_receive_request(NBDClient *client, NBDRequest 
*request,
 Error **errp)
 {
-uint8_t buf[NBD_REQUEST_SIZE];
-uint32_t magic;
+uint8_t buf[NBD_EXTENDED_REQUEST_SIZE];
+uint32_t magic, expect;
 int ret;
+size_t size = client->mode >= NBD_MODE_EXTENDED ?
+NBD_EXTENDED_REQUEST_SIZE : NBD_REQUEST_SIZE;

-ret = nbd_read_eof(client, buf, sizeof(buf), errp);
+ret = nbd_read_eof(client, buf, size, errp);
 if (ret < 0) {
 return ret;
 }
@@ -1423,13 +1425,21 @@ static int coroutine_fn nbd_receive_request(NBDClient 
*client, NBDRequest *reque
 return -EIO;
 }

-/* Request
-   [ 0 ..  3]   magic   (NBD_REQUEST_MAGIC)
-   [ 4 ..  5]   flags   (NBD_CMD_FLAG_FUA, ...)
-   [ 6 ..  7]   type(NBD_CMD_READ, ...)
-   [ 8 .. 15]   cookie
-   [16 .. 23]   from
-   [24 .. 27]   len
+/*
+ * Compact request
+ *  [ 0 ..  3]   magic   (NBD_REQUEST_MAGIC)
+ *  [ 4 ..  5]   flags   (NBD_CMD_FLAG_FUA, ...)
+ *  [ 6 ..  7]   type(NBD_CMD_READ, ...)
+ *  [ 8 .. 15]   cookie
+ *  [16 .. 23]   from
+ *  [24 .. 27]   len
+ * Extended request
+ *  [ 0 ..  3]   magic   (NBD_EXTENDED_REQUEST_MAGIC)
+ *  [ 4 ..  5]   flags   (NBD_CMD_FLAG_FUA, NBD_CMD_FLAG_PAYLOAD_LEN, ...)
+ *  [ 6 ..  7]   type(NBD_CMD_READ, ...)
+ *  [ 8 .. 15]   cookie
+ *  [16 .. 23]   from
+ *  [24 .. 31]   len
  */

 magic = ldl_be_p(buf);
@@ -1437,13 +1447,20 @@ static int coroutine_fn nbd_receive_request(NBDClient 
*client, NBDRequest *reque
 request->type   = lduw_be_p(buf + 6);
 request->cookie = ldq_be_p(buf + 8);
 request->from   = ldq_be_p(buf + 16);
-request->len= (uint32_t)ldl_be_p(buf + 24); /* widen 32 to 64 bits */
+if (client->mode >= NBD_MODE_EXTENDED) {
+request->len = ldq_be_p(buf + 24);
+expect = NBD_EXTENDED_REQUEST_MAGIC;
+} else {
+request->len = (uint32_t)ldl_be_p(buf + 24); /* widen 32 to 64 bits */
+expect = NBD_REQUEST_MAGIC;
+}

 trace_nbd_receive_request(magic, request->flags, request->type,
   request->from, request->len);

-if (magic != NBD_REQUEST_MAGIC) {
-error_setg(errp, "invalid magic (got 0x%" PRIx32 ")", magic);
+if (magic != expect) {
+error_setg(errp, "invalid magic (got 0x%" PRIx32 ", expected 0x%"
+   PRIx32 ")", magic, expect);
 return -EINVAL;
 }
 return 0;
-- 
2.41.0

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] [PATCH v7 00/12] NBD 64-bit extensions for qemu

[Eric Blake]

v6 was here:
https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg05231.html

Since then:
 - patches v6 1-5 included in pull request
 - patch v6 6 logic improved, now v7 patch 1
 - rebased to master

Still needing review:
 - patch 1,6,7,11,12

Eric Blake (12):
  nbd/server: Support a request payload
  nbd/server: Prepare to receive extended header requests
  nbd/server: Prepare to send extended header replies
  nbd/server: Support 64-bit block status
  nbd/server: Enable initial support for extended headers
  nbd/client: Plumb errp through nbd_receive_replies
  nbd/client: Initial support for extended headers
  nbd/client: Accept 64-bit block status chunks
  nbd/client: Request extended headers during negotiation
  nbd/server: Refactor list of negotiated meta contexts
  nbd/server: Prepare for per-request filtering of BLOCK_STATUS
  nbd/server: Add FLAG_PAYLOAD support to CMD_BLOCK_STATUS

 docs/interop/nbd.txt  |   1 +
 include/block/nbd.h   |   5 +-
 nbd/nbd-internal.h|   5 +-
 block/nbd.c   |  67 ++-
 nbd/client-connection.c   |   2 +-
 nbd/client.c  | 124 --
 nbd/server.c  | 418 ++
 qemu-nbd.c|   4 +
 block/trace-events|   1 +
 nbd/trace-events  |   5 +-
 tests/qemu-iotests/223.out|  18 +-
 tests/qemu-iotests/233.out|   4 +
 tests/qemu-iotests/241.out|   3 +
 tests/qemu-iotests/307.out|  15 +-
 .../tests/nbd-qemu-allocation.out |   3 +-
 15 files changed, 516 insertions(+), 159 deletions(-)

-- 
2.41.0

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] [PATCH v2v v2 6/6] convert: Find out if Windows guest is expecting RTC set to UTC

[Richard W.M. Jones]

Read HKLM\SYSTEM\CurrentControlSet\Control\TimeZoneInformation key
"RealTimeIsUniversal" to see if the Windows guest is expecting RTC
set to localtime (not present) or UTC (present and set to 1).

See: https://wiki.archlinux.org/title/System_time#UTC_in_Microsoft_Windows
See: 
https://listman.redhat.com/archives/libguestfs/2023-September/thread.html#32556
Reported-by: Lee Garrett
Reviewed-by: Laszlo Ersek 
---
 convert/convert_windows.ml | 38 +-
 tests/test-v2v-i-ova.xml   |  2 +-
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/convert/convert_windows.ml b/convert/convert_windows.ml
index 7e3aa8d789..34cf341b8e 100644
--- a/convert/convert_windows.ml
+++ b/convert/convert_windows.ml
@@ -103,6 +103,42 @@ let convert (g : G.guestfs) _ inspect i_firmware 
block_driver _ static_ips =
   (* If the Windows guest has AV installed. *)
   let has_antivirus = Windows.detect_antivirus inspect in
 
+  (* Does the guest expect the RTC to be set to UTC or localtime?
+   * See https://wiki.archlinux.org/title/System_time#UTC_in_Microsoft_Windows
+   * Note this might be a QWORD on 64 bit Windows instances.
+   *)
+  let rtc_utc =
+Registry.with_hive_readonly g inspect.i_windows_system_hive
+  (fun reg ->
+   try
+ let key_path = [ "Control"; "TimeZoneInformation" ] in
+ let path = inspect.i_windows_current_control_set :: key_path in
+ let node =
+   match Registry.get_node reg path with
+   | None -> raise Not_found
+   | Some node -> node in
+ let valueh = g#hivex_node_get_value node "RealTimeIsUniversal" in
+ if valueh = 0L then raise Not_found;
+ let t = g#hivex_value_type valueh in
+ let data = g#hivex_value_value valueh in
+ let is_utc =
+   match t with
+   | 0_L (* REG_NONE *) -> false (* localtime *)
+   | 4_L (* REG_DWORD *) -> data = "\001\000\000\000"
+   | 11_L (* REG_QWORD *) -> data = "\001\000\000\000\000\000\000\000"
+   | _ (* who knows ... *) ->
+ warning (f_"unknown CurrentControlSet\\Control\\\
+ TimeZoneInformation key RealTimeIsUniversal \
+ type 0x%Lx, assuming RTC set to UTC") t;
+ true in
+ is_utc
+   with Not_found ->
+ (* If the key is not found then by default we assume
+  * that Windows is expecting the RTC to be set to localtime.
+  *)
+ false
+  ) in
+
   (* Open the software hive (readonly) and find the Xen PV uninstaller,
* if it exists.
*)
@@ -275,7 +311,7 @@ let convert (g : G.guestfs) _ inspect i_firmware 
block_driver _ static_ips =
   gcaps_arch = Utils.kvm_arch inspect.i_arch;
   gcaps_arch_min_version = 0;
   gcaps_virtio_1_0 = virtio_win_installed.Inject_virtio_win.virtio_1_0;
-  gcaps_rtc_utc = true;
+  gcaps_rtc_utc = rtc_utc;
 } in
 
 guestcaps
diff --git a/tests/test-v2v-i-ova.xml b/tests/test-v2v-i-ova.xml
index a41827bfd5..fa7b4dbfc5 100644
--- a/tests/test-v2v-i-ova.xml
+++ b/tests/test-v2v-i-ova.xml
@@ -18,7 +18,7 @@
   
 hvm
   
-  
+  
   destroy
   restart
   restart
-- 
2.41.0

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] [PATCH v2v v2 3/6] output/create_libvirt_xml.ml: Refactor os_section

[Richard W.M. Jones]

Minor refactoring of how  section is generated in XML output.
There is no change in the output.

Reviewed-by: Laszlo Ersek 
---
 output/create_libvirt_xml.ml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml
index 964acd25fd..12586fde17 100644
--- a/output/create_libvirt_xml.ml
+++ b/output/create_libvirt_xml.ml
@@ -292,10 +292,10 @@ let create_libvirt_xml ?pool source inspect
e "nvram" ["template", vars_template] [] ] in
 
 List.push_back_list os loader;
-!os in
+e "os" [] !os in
 
   List.push_back_list body [
-e "os" [] os_section;
+os_section;
 
 e "on_poweroff" [] [PCData "destroy"];
 e "on_reboot" [] [PCData "restart"];
-- 
2.41.0

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] [PATCH v2v v2 4/6] -o libvirt: Add to libvirt XML

[Richard W.M. Jones]

Reviewed-by: Laszlo Ersek 
---
 output/create_libvirt_xml.ml | 6 ++
 tests/test-v2v-i-ova.xml | 1 +
 2 files changed, 7 insertions(+)

diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml
index 12586fde17..30119d1357 100644
--- a/output/create_libvirt_xml.ml
+++ b/output/create_libvirt_xml.ml
@@ -294,8 +294,14 @@ let create_libvirt_xml ?pool source inspect
 List.push_back_list os loader;
 e "os" [] !os in
 
+  (* The  section. *)
+  let clock_section =
+let offset = if guestcaps.gcaps_rtc_utc then "utc" else "localtime" in
+e "clock" [ "offset", offset ] [] in
+
   List.push_back_list body [
 os_section;
+clock_section;
 
 e "on_poweroff" [] [PCData "destroy"];
 e "on_reboot" [] [PCData "restart"];
diff --git a/tests/test-v2v-i-ova.xml b/tests/test-v2v-i-ova.xml
index e5907ea1cc..a41827bfd5 100644
--- a/tests/test-v2v-i-ova.xml
+++ b/tests/test-v2v-i-ova.xml
@@ -18,6 +18,7 @@
   
 hvm
   
+  
   destroy
   restart
   restart
-- 
2.41.0

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] [PATCH v2v v2 5/6] -o qemu: Set -rtc base=localtime when guest expects RTC set to localtime

[Richard W.M. Jones]

I didn't set the -rtc flag in the normal (UTC) case as that is the
default.

Reviewed-by: Laszlo Ersek 
---
 output/output_qemu.ml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/output/output_qemu.ml b/output/output_qemu.ml
index 57b1e58da2..1a5f7f7147 100644
--- a/output/output_qemu.ml
+++ b/output/output_qemu.ml
@@ -158,6 +158,8 @@ module QEMU = struct
 arg_list "-device" ["vmgenid"; sprintf "guid=%s" genid; "id=vmgenid0"]
 );
 
+if not guestcaps.gcaps_rtc_utc then arg "-rtc" "base=localtime";
+
 arg_list "-machine" (machine_str ::
(if smm then ["smm=on"] else []) @
["accel=kvm:tcg"]);
-- 
2.41.0

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] [[PATCH v2v v2 0/6] convert: Find out if Windows guest is expecting RTC set to UTC

[Richard W.M. Jones]

The main changes are:

 - BIOS -> RTC passim

 - Split out the minor refactoring patch

 - Enhanced commit messages

 - Add Laszlo's R-b tag

Rich.


___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] [PATCH v2v v2 1/6] types: Add gcaps_rtc_utc to record if the RTC is set to UTC or localtime

[Richard W.M. Jones]

Almost every Linux guest expects the motherboard Real Time Clock (RTC)
to be set to UTC and they adjust the time displayed based on their
timezone (which may be different for each user).

Most Windows guests expect the RTC to be set to the local time.
Windows can be configured to use a UTC clock.  We can detect this by
looking at the Windows registry.

To cope with this difference we need to add a guestcaps flag based on
what we think the guest is expecting.  (We might also use the source
hypervisor RTC setting, but it is not thought to be as reliable as
inspecting the guest.)

This change simply adds the flag to guestcaps, and sets it to always
true, so there is no change to the output.

Reviewed-by: Laszlo Ersek 
---
 lib/types.mli  | 5 +
 convert/convert_linux.ml   | 1 +
 convert/convert_windows.ml | 1 +
 lib/types.ml   | 3 +++
 4 files changed, 10 insertions(+)

diff --git a/lib/types.mli b/lib/types.mli
index 65ef2e35cf..3446bb64bd 100644
--- a/lib/types.mli
+++ b/lib/types.mli
@@ -277,6 +277,11 @@ type guestcaps = {
   gcaps_virtio_1_0 : bool;
   (** The guest supports the virtio devices that it does at the virtio-1.0
   protocol level. *)
+
+  gcaps_rtc_utc : bool;
+  (** Is the RTC set to UTC ([true]) or localtime ([false])?  For
+  Linux guests this is always true.  For Windows we find out
+  what the guest is expecting by looking at the registry. *)
 }
 (** Guest capabilities after conversion.  eg. Was virtio found or installed? *)
 
diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml
index 8d7020848f..bd54568b10 100644
--- a/convert/convert_linux.ml
+++ b/convert/convert_linux.ml
@@ -220,6 +220,7 @@ let convert (g : G.guestfs) source inspect i_firmware _ 
keep_serial_console _ =
   gcaps_arch = Utils.kvm_arch inspect.i_arch;
   gcaps_arch_min_version = arch_min_version;
   gcaps_virtio_1_0 = virtio_1_0;
+  gcaps_rtc_utc = true; (* almost all Linux expect RTC to be UTC *)
 } in
 
 guestcaps
diff --git a/convert/convert_windows.ml b/convert/convert_windows.ml
index 122d95469d..7e3aa8d789 100644
--- a/convert/convert_windows.ml
+++ b/convert/convert_windows.ml
@@ -275,6 +275,7 @@ let convert (g : G.guestfs) _ inspect i_firmware 
block_driver _ static_ips =
   gcaps_arch = Utils.kvm_arch inspect.i_arch;
   gcaps_arch_min_version = 0;
   gcaps_virtio_1_0 = virtio_win_installed.Inject_virtio_win.virtio_1_0;
+  gcaps_rtc_utc = true;
 } in
 
 guestcaps
diff --git a/lib/types.ml b/lib/types.ml
index 75c14fd4f6..d6f9a266c5 100644
--- a/lib/types.ml
+++ b/lib/types.ml
@@ -399,6 +399,7 @@ type guestcaps = {
   gcaps_arch : string;
   gcaps_arch_min_version : int;
   gcaps_virtio_1_0 : bool;
+  gcaps_rtc_utc : bool;
 }
 and guestcaps_block_type = Virtio_blk | Virtio_SCSI | IDE
 and guestcaps_net_type = Virtio_net | E1000 | RTL8139
@@ -429,6 +430,7 @@ let string_of_guestcaps gcaps =
gcaps_arch = %s\n\
gcaps_arch_min_version = %d\n\
gcaps_virtio_1_0 = %b\n\
+   gcaps_rtc_utc = %b\n\
   "
   (string_of_block_type gcaps.gcaps_block_bus)
   (string_of_net_type gcaps.gcaps_net_bus)
@@ -440,6 +442,7 @@ let string_of_guestcaps gcaps =
   gcaps.gcaps_arch
   gcaps.gcaps_arch_min_version
   gcaps.gcaps_virtio_1_0
+  gcaps.gcaps_rtc_utc
 
 type target_buses = {
   target_virtio_blk_bus : target_bus_slot array;
-- 
2.41.0

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] [PATCH v2v v2 2/6] -o kubevirt: Add comment about future support for clock = localtime

[Richard W.M. Jones]

Reviewed-by: Laszlo Ersek 
---
 output/create_kubevirt_yaml.ml | 5 +
 1 file changed, 5 insertions(+)

diff --git a/output/create_kubevirt_yaml.ml b/output/create_kubevirt_yaml.ml
index 689555e4dc..e3a3f422df 100644
--- a/output/create_kubevirt_yaml.ml
+++ b/output/create_kubevirt_yaml.ml
@@ -54,6 +54,11 @@ let create_kubevirt_yaml source inspect
   "pit", Assoc [ "tickPolicy", String "delay" ];
   "rtc", Assoc [ "tickPolicy", String "catchup" ];
 ];
+(* XXX Note that we may need to set "localtime" here
+ * depending on guestcaps.gcaps_rtc_utc.  However that
+ * requires the following PR to be merged in Kubevirt:
+ * https://github.com/kubevirt/kubevirt/pull/9587
+ *)
 "utc", List []
   ]
 )
-- 
2.41.0

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v2v 5/5] convert: Find out if Windows guest is expected BIOS set to localtime

[Richard W.M. Jones]

On Mon, Sep 25, 2023 at 05:58:00PM +0200, Laszlo Ersek wrote:
> On 9/25/23 16:04, Richard W.M. Jones wrote:
> > Read HKLM\SYSTEM\CurrentControlSet\Control\TimeZoneInformation key
> > "RealTimeIsUniversal" to see if the Windows guest is expecting BIOS
> > set to localtime (not present) or UTC (present and set to 1).
> > 
> > See: https://wiki.archlinux.org/title/System_time#UTC_in_Microsoft_Windows
> > See: 
> > https://listman.redhat.com/archives/libguestfs/2023-September/thread.html#32556
> > Reported-by: Lee Garrett
> > ---
> >  convert/convert_windows.ml | 38 +-
> >  tests/test-v2v-i-ova.xml   |  2 +-
> >  2 files changed, 38 insertions(+), 2 deletions(-)
> > 
> > diff --git a/convert/convert_windows.ml b/convert/convert_windows.ml
> > index f6e039be7e..84e8f7b7d3 100644
> > --- a/convert/convert_windows.ml
> > +++ b/convert/convert_windows.ml
> > @@ -103,6 +103,42 @@ let convert (g : G.guestfs) _ inspect i_firmware 
> > block_driver _ static_ips =
> >(* If the Windows guest has AV installed. *)
> >let has_antivirus = Windows.detect_antivirus inspect in
> >  
> > +  (* Does the guest expect the BIOS to be set to UTC or localtime?
> > +   * See 
> > https://wiki.archlinux.org/title/System_time#UTC_in_Microsoft_Windows
> > +   * Note this might be a QWORD on 64 bit Windows instances.
> > +   *)
> > +  let bios_utc =
> > +Registry.with_hive_readonly g inspect.i_windows_system_hive
> > +  (fun reg ->
> > +   try
> > + let key_path = [ "Control"; "TimeZoneInformation" ] in
> > + let path = inspect.i_windows_current_control_set :: key_path in
> > + let node =
> > +   match Registry.get_node reg path with
> > +   | None -> raise Not_found
> > +   | Some node -> node in
> > + let valueh = g#hivex_node_get_value node "RealTimeIsUniversal" in
> > + if valueh = 0L then raise Not_found;
> > + let t = g#hivex_value_type valueh in
> > + let data = g#hivex_value_value valueh in
> > + let is_utc =
> > +   match t with
> > +   | 0_L (* REG_NONE *) -> false (* localtime *)
> > +   | 4_L (* REG_DWORD *) -> data = "\001\000\000\000"
> > +   | 11_L (* REG_QWORD *) -> data = 
> > "\001\000\000\000\000\000\000\000"
> > +   | _ (* who knows ... *) ->
> > + warning (f_"unknown CurrentControlSet\\Control\\\
> > + TimeZoneInformation key RealTimeIsUniversal \
> > + type 0x%Lx, assuming BIOS set to UTC") t;
> > + true in
> 
> [*]
> 
> > + is_utc
> > +   with Not_found ->
> > + (* If the key is not found then by default we assume
> > +  * that Windows is expecting the BIOS to be set to localtime.
> > +  *)
> > + false
> > +  ) in
> > +
> >(* Open the software hive (readonly) and find the Xen PV uninstaller,
> > * if it exists.
> > *)
> 
> [*] Just guessing, but I'm not sure if assuming UTC is right when the
> key is found but its type is unrecognized, in light of determining
> localtime when the key is absent or has type REG_NONE.
> 
> Either way, the warning is honest about it!

I don't know - I would need to examine lots of Windows machines to
find out what they really do.  But I expect that this field only ever
contains dword:1 or hex(b):1 [qword:1], or is absent.

> > @@ -275,7 +311,7 @@ let convert (g : G.guestfs) _ inspect i_firmware 
> > block_driver _ static_ips =
> >gcaps_arch = Utils.kvm_arch inspect.i_arch;
> >gcaps_arch_min_version = 0;
> >gcaps_virtio_1_0 = virtio_win_installed.Inject_virtio_win.virtio_1_0;
> > -  gcaps_bios_utc = true;
> > +  gcaps_bios_utc = bios_utc;
> >  } in
> >  
> >  guestcaps
> > diff --git a/tests/test-v2v-i-ova.xml b/tests/test-v2v-i-ova.xml
> > index a41827bfd5..fa7b4dbfc5 100644
> > --- a/tests/test-v2v-i-ova.xml
> > +++ b/tests/test-v2v-i-ova.xml
> > @@ -18,7 +18,7 @@
> >
> >  hvm
> >
> > -  
> > +  
> >destroy
> >restart
> >restart
> 
> With the BIOS -> RTC typo (?) fixed everywhere:
> 
> series
> Reviewed-by: Laszlo Ersek 

Thanks, I'll post v2 anyway since I've written it now.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v2v 5/5] convert: Find out if Windows guest is expected BIOS set to localtime

[Laszlo Ersek]

On 9/25/23 16:04, Richard W.M. Jones wrote:
> Read HKLM\SYSTEM\CurrentControlSet\Control\TimeZoneInformation key
> "RealTimeIsUniversal" to see if the Windows guest is expecting BIOS
> set to localtime (not present) or UTC (present and set to 1).
> 
> See: https://wiki.archlinux.org/title/System_time#UTC_in_Microsoft_Windows
> See: 
> https://listman.redhat.com/archives/libguestfs/2023-September/thread.html#32556
> Reported-by: Lee Garrett
> ---
>  convert/convert_windows.ml | 38 +-
>  tests/test-v2v-i-ova.xml   |  2 +-
>  2 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/convert/convert_windows.ml b/convert/convert_windows.ml
> index f6e039be7e..84e8f7b7d3 100644
> --- a/convert/convert_windows.ml
> +++ b/convert/convert_windows.ml
> @@ -103,6 +103,42 @@ let convert (g : G.guestfs) _ inspect i_firmware 
> block_driver _ static_ips =
>(* If the Windows guest has AV installed. *)
>let has_antivirus = Windows.detect_antivirus inspect in
>  
> +  (* Does the guest expect the BIOS to be set to UTC or localtime?
> +   * See 
> https://wiki.archlinux.org/title/System_time#UTC_in_Microsoft_Windows
> +   * Note this might be a QWORD on 64 bit Windows instances.
> +   *)
> +  let bios_utc =
> +Registry.with_hive_readonly g inspect.i_windows_system_hive
> +  (fun reg ->
> +   try
> + let key_path = [ "Control"; "TimeZoneInformation" ] in
> + let path = inspect.i_windows_current_control_set :: key_path in
> + let node =
> +   match Registry.get_node reg path with
> +   | None -> raise Not_found
> +   | Some node -> node in
> + let valueh = g#hivex_node_get_value node "RealTimeIsUniversal" in
> + if valueh = 0L then raise Not_found;
> + let t = g#hivex_value_type valueh in
> + let data = g#hivex_value_value valueh in
> + let is_utc =
> +   match t with
> +   | 0_L (* REG_NONE *) -> false (* localtime *)
> +   | 4_L (* REG_DWORD *) -> data = "\001\000\000\000"
> +   | 11_L (* REG_QWORD *) -> data = 
> "\001\000\000\000\000\000\000\000"
> +   | _ (* who knows ... *) ->
> + warning (f_"unknown CurrentControlSet\\Control\\\
> + TimeZoneInformation key RealTimeIsUniversal \
> + type 0x%Lx, assuming BIOS set to UTC") t;
> + true in

[*]

> + is_utc
> +   with Not_found ->
> + (* If the key is not found then by default we assume
> +  * that Windows is expecting the BIOS to be set to localtime.
> +  *)
> + false
> +  ) in
> +
>(* Open the software hive (readonly) and find the Xen PV uninstaller,
> * if it exists.
> *)

[*] Just guessing, but I'm not sure if assuming UTC is right when the
key is found but its type is unrecognized, in light of determining
localtime when the key is absent or has type REG_NONE.

Either way, the warning is honest about it!

> @@ -275,7 +311,7 @@ let convert (g : G.guestfs) _ inspect i_firmware 
> block_driver _ static_ips =
>gcaps_arch = Utils.kvm_arch inspect.i_arch;
>gcaps_arch_min_version = 0;
>gcaps_virtio_1_0 = virtio_win_installed.Inject_virtio_win.virtio_1_0;
> -  gcaps_bios_utc = true;
> +  gcaps_bios_utc = bios_utc;
>  } in
>  
>  guestcaps
> diff --git a/tests/test-v2v-i-ova.xml b/tests/test-v2v-i-ova.xml
> index a41827bfd5..fa7b4dbfc5 100644
> --- a/tests/test-v2v-i-ova.xml
> +++ b/tests/test-v2v-i-ova.xml
> @@ -18,7 +18,7 @@
>
>  hvm
>
> -  
> +  
>destroy
>restart
>restart

With the BIOS -> RTC typo (?) fixed everywhere:

series
Reviewed-by: Laszlo Ersek 

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v2v 0/5] convert: Find out if Windows guest is expecting BIOS localtime or UTC

[Richard W.M. Jones]

On Mon, Sep 25, 2023 at 05:49:18PM +0200, Laszlo Ersek wrote:
> On 9/25/23 17:48, Laszlo Ersek wrote:
> > On 9/25/23 16:04, Richard W.M. Jones wrote:
> >> [Alice: See patch 2]
> >>
> >> [This patch is a bit rough, it could do with better commit messages
> >> and some tests.  Please test it to see if it solves the Windows
> >> conversion issue described in the thread below.]
> >>
> >> We currently do not set any  field in guest output.  Most
> >> Windows guests expect the BIOS to be set to localtime, whereas almost
> >> all Linux guests would expect it to be set to UTC.  It is also
> >> possible to configure a Windows guest to expect BIOS set to UTC.
> >>
> >> The default is usually BIOS set to UTC, so for many Windows guests
> >> this would be wrong.  This specifically may cause problems when
> >> scheduling qemu-ga installation, see the thread here:
> >>
> >> https://listman.redhat.com/archives/libguestfs/2023-September/thread.html#32556
> >>
> >> but could cause other general issues with time in the guest.
> >>
> >> One way to implement this would be to copy the source hypervisor
> >> information across; however I'm not confident this information is read
> >> correctly.  A better way is to read out what the guest is expecting
> >> from the Windows registry.  (For Linux we just assume BIOS is always
> >> UTC, since that's the default for almost any Linux guest which hasn't
> >> been dual-booted with Windows, which for VMs would be incredibly
> >> rare.)
> > 
> > I think the word "BIOS" is incorrectly used all over the series; I'd
> > rather say "RTC" / "real time clock".
> 
> ... meaning patch subject lines, commit message bodies, and code.

OK :-)

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v2v 3/5] -o libvirt: Add to libvirt XML

[Richard W.M. Jones]

On Mon, Sep 25, 2023 at 05:47:26PM +0200, Laszlo Ersek wrote:
> On 9/25/23 16:04, Richard W.M. Jones wrote:
> > ---
> >  output/create_libvirt_xml.ml | 10 --
> >  tests/test-v2v-i-ova.xml |  1 +
> >  2 files changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml
> > index 964acd25fd..f97272ca31 100644
> > --- a/output/create_libvirt_xml.ml
> > +++ b/output/create_libvirt_xml.ml
> > @@ -292,10 +292,16 @@ let create_libvirt_xml ?pool source inspect
> > e "nvram" ["template", vars_template] [] ] in
> >  
> >  List.push_back_list os loader;
> > -!os in
> > +e "os" [] !os in
> > +
> > +  (* The  section. *)
> > +  let clock_section =
> > +let offset = if guestcaps.gcaps_bios_utc then "utc" else "localtime" in
> > +e "clock" [ "offset", offset ] [] in
> >  
> >List.push_back_list body [
> > -e "os" [] os_section;
> > +os_section;
> > +clock_section;
> >  
> >  e "on_poweroff" [] [PCData "destroy"];
> >  e "on_reboot" [] [PCData "restart"];
> 
> I don't think we need to change what "os_section" is bound to, here; I
> understand the idea is to increase consistency, but for me it makes the
> patch harder to read -- ultimately it is a refactoring, and then adding
> in clock_section is the new thing.
> 
> If you can split these apart, that's optimal; if not, this is viable too
> IMO.

Yup, that is a separate clean-up, will split.

Rich.

> Thanks
> Laszlo
> 
> > diff --git a/tests/test-v2v-i-ova.xml b/tests/test-v2v-i-ova.xml
> > index e5907ea1cc..a41827bfd5 100644
> > --- a/tests/test-v2v-i-ova.xml
> > +++ b/tests/test-v2v-i-ova.xml
> > @@ -18,6 +18,7 @@
> >
> >  hvm
> >
> > +  
> >destroy
> >restart
> >restart

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v2v 0/5] convert: Find out if Windows guest is expecting BIOS localtime or UTC

[Laszlo Ersek]

On 9/25/23 17:48, Laszlo Ersek wrote:
> On 9/25/23 16:04, Richard W.M. Jones wrote:
>> [Alice: See patch 2]
>>
>> [This patch is a bit rough, it could do with better commit messages
>> and some tests.  Please test it to see if it solves the Windows
>> conversion issue described in the thread below.]
>>
>> We currently do not set any  field in guest output.  Most
>> Windows guests expect the BIOS to be set to localtime, whereas almost
>> all Linux guests would expect it to be set to UTC.  It is also
>> possible to configure a Windows guest to expect BIOS set to UTC.
>>
>> The default is usually BIOS set to UTC, so for many Windows guests
>> this would be wrong.  This specifically may cause problems when
>> scheduling qemu-ga installation, see the thread here:
>>
>> https://listman.redhat.com/archives/libguestfs/2023-September/thread.html#32556
>>
>> but could cause other general issues with time in the guest.
>>
>> One way to implement this would be to copy the source hypervisor
>> information across; however I'm not confident this information is read
>> correctly.  A better way is to read out what the guest is expecting
>> from the Windows registry.  (For Linux we just assume BIOS is always
>> UTC, since that's the default for almost any Linux guest which hasn't
>> been dual-booted with Windows, which for VMs would be incredibly
>> rare.)
> 
> I think the word "BIOS" is incorrectly used all over the series; I'd
> rather say "RTC" / "real time clock".

... meaning patch subject lines, commit message bodies, and code.

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v2v 0/5] convert: Find out if Windows guest is expecting BIOS localtime or UTC

[Laszlo Ersek]

On 9/25/23 16:04, Richard W.M. Jones wrote:
> [Alice: See patch 2]
> 
> [This patch is a bit rough, it could do with better commit messages
> and some tests.  Please test it to see if it solves the Windows
> conversion issue described in the thread below.]
> 
> We currently do not set any  field in guest output.  Most
> Windows guests expect the BIOS to be set to localtime, whereas almost
> all Linux guests would expect it to be set to UTC.  It is also
> possible to configure a Windows guest to expect BIOS set to UTC.
> 
> The default is usually BIOS set to UTC, so for many Windows guests
> this would be wrong.  This specifically may cause problems when
> scheduling qemu-ga installation, see the thread here:
> 
> https://listman.redhat.com/archives/libguestfs/2023-September/thread.html#32556
> 
> but could cause other general issues with time in the guest.
> 
> One way to implement this would be to copy the source hypervisor
> information across; however I'm not confident this information is read
> correctly.  A better way is to read out what the guest is expecting
> from the Windows registry.  (For Linux we just assume BIOS is always
> UTC, since that's the default for almost any Linux guest which hasn't
> been dual-booted with Windows, which for VMs would be incredibly
> rare.)

I think the word "BIOS" is incorrectly used all over the series; I'd
rather say "RTC" / "real time clock".

Laszlo

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v2v 3/5] -o libvirt: Add to libvirt XML

[Laszlo Ersek]

On 9/25/23 16:04, Richard W.M. Jones wrote:
> ---
>  output/create_libvirt_xml.ml | 10 --
>  tests/test-v2v-i-ova.xml |  1 +
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml
> index 964acd25fd..f97272ca31 100644
> --- a/output/create_libvirt_xml.ml
> +++ b/output/create_libvirt_xml.ml
> @@ -292,10 +292,16 @@ let create_libvirt_xml ?pool source inspect
> e "nvram" ["template", vars_template] [] ] in
>  
>  List.push_back_list os loader;
> -!os in
> +e "os" [] !os in
> +
> +  (* The  section. *)
> +  let clock_section =
> +let offset = if guestcaps.gcaps_bios_utc then "utc" else "localtime" in
> +e "clock" [ "offset", offset ] [] in
>  
>List.push_back_list body [
> -e "os" [] os_section;
> +os_section;
> +clock_section;
>  
>  e "on_poweroff" [] [PCData "destroy"];
>  e "on_reboot" [] [PCData "restart"];

I don't think we need to change what "os_section" is bound to, here; I
understand the idea is to increase consistency, but for me it makes the
patch harder to read -- ultimately it is a refactoring, and then adding
in clock_section is the new thing.

If you can split these apart, that's optimal; if not, this is viable too
IMO.

Thanks
Laszlo

> diff --git a/tests/test-v2v-i-ova.xml b/tests/test-v2v-i-ova.xml
> index e5907ea1cc..a41827bfd5 100644
> --- a/tests/test-v2v-i-ova.xml
> +++ b/tests/test-v2v-i-ova.xml
> @@ -18,6 +18,7 @@
>
>  hvm
>
> +  
>destroy
>restart
>restart

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v2v 0/5] convert: Find out if Windows guest is expecting BIOS localtime or UTC

[Daniel P . Berrangé]

On Mon, Sep 25, 2023 at 04:26:59PM +0200, Alice Frosi wrote:
> Hi Rich,
> 
> On Mon, Sep 25, 2023 at 4:10 PM Richard W.M. Jones 
> wrote:
> 
> > [Alice: See patch 2]
> >
> 
> I'm not 100% sure about the source of this work. However, we had in
> KubeVirt people interested in using localtime with Windows [1]. Yes, I see
> that you pointed to that PR in patch 2. The problem with that PR is the
> migration. What happens if we migrate the VM to a host that is in another
> timezone? Is it going to break the application running inside the VM?

I would generally expect all hosts to be configured in the same timezone,
for any given single cloud deployment.

If you really are concerned about this problem though, libvirt lets you
set this explicitly eg

   

With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [EXTERNAL] - Re: [EXTERNAL] - Re: Libguestfs desired version installation

[Raja Ram Sharma]

Hi Richard/Eric, 

I wanted to take a moment to express my sincere gratitude for your invaluable 
contributions to "libguestfs" project.
And assistance with current issue "mount_local: unknown option 684826080".
 
The issue has been successfully resolved and our project is back on track.

Just to give one glimpse of what was error : It was a silly mistake while using 
library. 
We were not passing -1 as last parameter to guestfs_mount_local(guestfs *, 
basedir, -1);
due to which error was not consistent.

Your commitment to the open-source community is truly commendable. Your 
willingness to share your time, knowledge, and skills with others has a 
profound impact on community. Your guidance and support not only helped us 
overcome a technical challenge but also enriched our understanding of 
"libguesfs" project.

If there is anything we can do to support your projects or initiatives, please 
do not hesitate to let us know. Your work is essential, and we are here to 
assist in any way we can.

Once again, we greatly appreciate your help and look forward to many more 
collaboration in the future.

Marking this mail to an end milestone. Please forgive for any un-comfortability 
caused by us.



Thanks
Raja Ram (ArbitCode)



-Original Message-
From: Eric Blake  
Sent: Monday, September 25, 2023 7:39 PM
To: Richard W.M. Jones 
Cc: Teja Konapalli ; Divyanshu Kumar 
; libguestfs@redhat.com; Raja Ram Sharma 

Subject: [EXTERNAL] - Re: [Libguestfs] [EXTERNAL] - Re: Libguestfs desired 
version installation

CAUTION: This email originated from outside of the organization. Do not click 
links or open attachments unless you recognize the sender and know the content 
is safe. If you feel that the email is suspicious, please report it using 
PhishAlarm.


On Mon, Sep 25, 2023 at 10:16:10AM +0100, Richard W.M. Jones wrote:
> On Mon, Sep 25, 2023 at 09:09:32AM +, Teja Konapalli wrote:
> > Hi Richard,
> >
> > Yes, you are correct my ask is without upgrading OS version(from 8.2 
> > to 9.0) is it possible to upgrade libguestfs alone to different 
> > versions.
>
> No, that is not supported.

Worded differently:

Libguestfs is open source.  Therefore, you have the freedom to install whatever 
libguestfs version you want on whatever underlying system you want - and it may 
even work the way you want.  However, if you choose to install a version of 
libguestfs on a distro that is different than the version that your distro 
already provides, then you will be stuck debugging any issues that arise on 
your own, and cannot reasonably expect either your distro or upstream to help 
you (your distro's answer will likely be "use the version we already debugged 
for you").

Upstream support is a limited resource - there are no contractual guarantees 
that anyone has to help you, and while it may be low cost when it works in your 
favor, you are far more likely to get responses based on the quality of effort 
you have put in to reporting problems accurately, rather than expecting others 
to do your homework for you.

You mention using RHEL; since you are already paying to use a distro, you might 
as well make the most of your support contract.  However, this is the upstream 
mailing list, and not the point of contact with your RHEL support contract.  
Then as Rich has already mentioned:

> >
> > Since you have (still) not described the exact environment you're using, or 
> > even accurately described the problem you're having, I don't know if this 
> > will make any difference.

your distro support contract will likewise expect you to put in more effort of 
describing the actual scenario you are trying to solve, rather than the 
secondary problem of "can I upgrade libguestfs" that may or may not address the 
primary problem that you are facing.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v2v 0/5] convert: Find out if Windows guest is expecting BIOS localtime or UTC

[Alice Frosi]

Hi Rich,

On Mon, Sep 25, 2023 at 4:10 PM Richard W.M. Jones 
wrote:

> [Alice: See patch 2]
>

I'm not 100% sure about the source of this work. However, we had in
KubeVirt people interested in using localtime with Windows [1]. Yes, I see
that you pointed to that PR in patch 2. The problem with that PR is the
migration. What happens if we migrate the VM to a host that is in another
timezone? Is it going to break the application running inside the VM?

IMO, that was the major blocker of that PR. I initially suggested that we
either avoid migration as a first step or try to migrate only on nodes at
the same timezone,
Do you know if anyone has experience on this particular problem?

[1] https://github.com/kubevirt/kubevirt/pull/9587

Many thanks,
Alice
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v2v 0/5] convert: Find out if Windows guest is expecting BIOS localtime or UTC

[Richard W.M. Jones]

On Mon, Sep 25, 2023 at 04:26:59PM +0200, Alice Frosi wrote:
> Hi Rich,
> 
> On Mon, Sep 25, 2023 at 4:10 PM Richard W.M. Jones  wrote:
> 
> [Alice: See patch 2]
> 
>
> I'm not 100% sure about the source of this work. However, we had in
> KubeVirt people interested in using localtime with Windows [1]. Yes,
> I see that you pointed to that PR in patch 2. The problem with that
> PR is the migration. What happens if we migrate the VM to a host
> that is in another timezone? Is it going to break the application
> running inside the VM?

Does every Windows VM running in Kubevirt have its registry adjusted
so it expects the BIOS to be set to UTC?  (ie:
https://wiki.archlinux.org/title/System_time#UTC_in_Microsoft_Windows).
This is a non-standard setting, but I can't understand how Windows VMs
would have correct clocks if this were not the case.

BTW you could find out by running:

  $ virt-win-reg window.img 'HKLM\SYSTEM'

on some Kubevirt Windows VMs, and looking for:

  [\ControlSet001\Control\TimeZoneInformation]

in the output.  See if there is a key called "RealTimeIsUniversal" and
what it is set to.

If it is the case that this is always set for existing Kubevirt
guests, then virt-v2v would have to be changed so it also sets this
registry entry when the output is Kubevirt.

I agree that migration across timezones is highly problematic for the
BIOS = localtime situation.  I don't see any solution for that except
disallowing it.

> IMO, that was the major blocker of that PR. I initially suggested that we
> either avoid migration as a first step or try to migrate only on nodes at the
> same timezone,
> Do you know if anyone has experience on this particular problem?

I guess RHV must have encountered this before, but I don't know any
specifics.

Thanks,

Rich.

> [1] https://github.com/kubevirt/kubevirt/pull/9587
> 
> Many thanks,
> Alice

-- 
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
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] [PATCH v2v 0/5] convert: Find out if Windows guest is expecting BIOS localtime or UTC

[Richard W.M. Jones]

[Alice: See patch 2]

[This patch is a bit rough, it could do with better commit messages
and some tests.  Please test it to see if it solves the Windows
conversion issue described in the thread below.]

We currently do not set any  field in guest output.  Most
Windows guests expect the BIOS to be set to localtime, whereas almost
all Linux guests would expect it to be set to UTC.  It is also
possible to configure a Windows guest to expect BIOS set to UTC.

The default is usually BIOS set to UTC, so for many Windows guests
this would be wrong.  This specifically may cause problems when
scheduling qemu-ga installation, see the thread here:

https://listman.redhat.com/archives/libguestfs/2023-September/thread.html#32556

but could cause other general issues with time in the guest.

One way to implement this would be to copy the source hypervisor
information across; however I'm not confident this information is read
correctly.  A better way is to read out what the guest is expecting
from the Windows registry.  (For Linux we just assume BIOS is always
UTC, since that's the default for almost any Linux guest which hasn't
been dual-booted with Windows, which for VMs would be incredibly
rare.)

Rich.


___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] [PATCH v2v 5/5] convert: Find out if Windows guest is expected BIOS set to localtime

[Richard W.M. Jones]

Read HKLM\SYSTEM\CurrentControlSet\Control\TimeZoneInformation key
"RealTimeIsUniversal" to see if the Windows guest is expecting BIOS
set to localtime (not present) or UTC (present and set to 1).

See: https://wiki.archlinux.org/title/System_time#UTC_in_Microsoft_Windows
See: 
https://listman.redhat.com/archives/libguestfs/2023-September/thread.html#32556
Reported-by: Lee Garrett
---
 convert/convert_windows.ml | 38 +-
 tests/test-v2v-i-ova.xml   |  2 +-
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/convert/convert_windows.ml b/convert/convert_windows.ml
index f6e039be7e..84e8f7b7d3 100644
--- a/convert/convert_windows.ml
+++ b/convert/convert_windows.ml
@@ -103,6 +103,42 @@ let convert (g : G.guestfs) _ inspect i_firmware 
block_driver _ static_ips =
   (* If the Windows guest has AV installed. *)
   let has_antivirus = Windows.detect_antivirus inspect in
 
+  (* Does the guest expect the BIOS to be set to UTC or localtime?
+   * See https://wiki.archlinux.org/title/System_time#UTC_in_Microsoft_Windows
+   * Note this might be a QWORD on 64 bit Windows instances.
+   *)
+  let bios_utc =
+Registry.with_hive_readonly g inspect.i_windows_system_hive
+  (fun reg ->
+   try
+ let key_path = [ "Control"; "TimeZoneInformation" ] in
+ let path = inspect.i_windows_current_control_set :: key_path in
+ let node =
+   match Registry.get_node reg path with
+   | None -> raise Not_found
+   | Some node -> node in
+ let valueh = g#hivex_node_get_value node "RealTimeIsUniversal" in
+ if valueh = 0L then raise Not_found;
+ let t = g#hivex_value_type valueh in
+ let data = g#hivex_value_value valueh in
+ let is_utc =
+   match t with
+   | 0_L (* REG_NONE *) -> false (* localtime *)
+   | 4_L (* REG_DWORD *) -> data = "\001\000\000\000"
+   | 11_L (* REG_QWORD *) -> data = "\001\000\000\000\000\000\000\000"
+   | _ (* who knows ... *) ->
+ warning (f_"unknown CurrentControlSet\\Control\\\
+ TimeZoneInformation key RealTimeIsUniversal \
+ type 0x%Lx, assuming BIOS set to UTC") t;
+ true in
+ is_utc
+   with Not_found ->
+ (* If the key is not found then by default we assume
+  * that Windows is expecting the BIOS to be set to localtime.
+  *)
+ false
+  ) in
+
   (* Open the software hive (readonly) and find the Xen PV uninstaller,
* if it exists.
*)
@@ -275,7 +311,7 @@ let convert (g : G.guestfs) _ inspect i_firmware 
block_driver _ static_ips =
   gcaps_arch = Utils.kvm_arch inspect.i_arch;
   gcaps_arch_min_version = 0;
   gcaps_virtio_1_0 = virtio_win_installed.Inject_virtio_win.virtio_1_0;
-  gcaps_bios_utc = true;
+  gcaps_bios_utc = bios_utc;
 } in
 
 guestcaps
diff --git a/tests/test-v2v-i-ova.xml b/tests/test-v2v-i-ova.xml
index a41827bfd5..fa7b4dbfc5 100644
--- a/tests/test-v2v-i-ova.xml
+++ b/tests/test-v2v-i-ova.xml
@@ -18,7 +18,7 @@
   
 hvm
   
-  
+  
   destroy
   restart
   restart
-- 
2.41.0

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] [PATCH v2v 2/5] -o kubevirt: Add comment about future support for clock = localtime

[Richard W.M. Jones]

---
 output/create_kubevirt_yaml.ml | 5 +
 1 file changed, 5 insertions(+)

diff --git a/output/create_kubevirt_yaml.ml b/output/create_kubevirt_yaml.ml
index 689555e4dc..b96c742832 100644
--- a/output/create_kubevirt_yaml.ml
+++ b/output/create_kubevirt_yaml.ml
@@ -54,6 +54,11 @@ let create_kubevirt_yaml source inspect
   "pit", Assoc [ "tickPolicy", String "delay" ];
   "rtc", Assoc [ "tickPolicy", String "catchup" ];
 ];
+(* XXX Note that we may need to set "localtime" here
+ * depending on guestcaps.gcaps_bios_utc.  However that
+ * requires the following PR to be merged in Kubevirt:
+ * https://github.com/kubevirt/kubevirt/pull/9587
+ *)
 "utc", List []
   ]
 )
-- 
2.41.0

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] [PATCH v2v 4/5] -o qemu: Set -rtc base=localtime when guest expects BIOS set to localtime

[Richard W.M. Jones]

I didn't set the -rtc flag in the normal (UTC) case as that is the
default.
---
 output/output_qemu.ml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/output/output_qemu.ml b/output/output_qemu.ml
index 57b1e58da2..a9d7b37071 100644
--- a/output/output_qemu.ml
+++ b/output/output_qemu.ml
@@ -158,6 +158,8 @@ module QEMU = struct
 arg_list "-device" ["vmgenid"; sprintf "guid=%s" genid; "id=vmgenid0"]
 );
 
+if not guestcaps.gcaps_bios_utc then arg "-rtc" "base=localtime";
+
 arg_list "-machine" (machine_str ::
(if smm then ["smm=on"] else []) @
["accel=kvm:tcg"]);
-- 
2.41.0

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] [PATCH v2v 3/5] -o libvirt: Add to libvirt XML

[Richard W.M. Jones]

---
 output/create_libvirt_xml.ml | 10 --
 tests/test-v2v-i-ova.xml |  1 +
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml
index 964acd25fd..f97272ca31 100644
--- a/output/create_libvirt_xml.ml
+++ b/output/create_libvirt_xml.ml
@@ -292,10 +292,16 @@ let create_libvirt_xml ?pool source inspect
e "nvram" ["template", vars_template] [] ] in
 
 List.push_back_list os loader;
-!os in
+e "os" [] !os in
+
+  (* The  section. *)
+  let clock_section =
+let offset = if guestcaps.gcaps_bios_utc then "utc" else "localtime" in
+e "clock" [ "offset", offset ] [] in
 
   List.push_back_list body [
-e "os" [] os_section;
+os_section;
+clock_section;
 
 e "on_poweroff" [] [PCData "destroy"];
 e "on_reboot" [] [PCData "restart"];
diff --git a/tests/test-v2v-i-ova.xml b/tests/test-v2v-i-ova.xml
index e5907ea1cc..a41827bfd5 100644
--- a/tests/test-v2v-i-ova.xml
+++ b/tests/test-v2v-i-ova.xml
@@ -18,6 +18,7 @@
   
 hvm
   
+  
   destroy
   restart
   restart
-- 
2.41.0

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] [PATCH v2v 1/5] types: Add gcaps_bios_utc to record if the BIOS is set to UTC or localtime

[Richard W.M. Jones]

This change simply adds the flag to guestcaps, and sets it to always
true, so there is no change to the output.
---
 lib/types.mli  | 5 +
 convert/convert_linux.ml   | 1 +
 convert/convert_windows.ml | 1 +
 lib/types.ml   | 3 +++
 4 files changed, 10 insertions(+)

diff --git a/lib/types.mli b/lib/types.mli
index 65ef2e35cf..44b35faeb0 100644
--- a/lib/types.mli
+++ b/lib/types.mli
@@ -277,6 +277,11 @@ type guestcaps = {
   gcaps_virtio_1_0 : bool;
   (** The guest supports the virtio devices that it does at the virtio-1.0
   protocol level. *)
+
+  gcaps_bios_utc : bool;
+  (** Is the BIOS set to UTC ([true]) or localtime ([false])?  For
+  Linux guests this is always true.  For Windows we find out
+  what the guest is expecting by looking at the registry. *)
 }
 (** Guest capabilities after conversion.  eg. Was virtio found or installed? *)
 
diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml
index 8d7020848f..a70d43224c 100644
--- a/convert/convert_linux.ml
+++ b/convert/convert_linux.ml
@@ -220,6 +220,7 @@ let convert (g : G.guestfs) source inspect i_firmware _ 
keep_serial_console _ =
   gcaps_arch = Utils.kvm_arch inspect.i_arch;
   gcaps_arch_min_version = arch_min_version;
   gcaps_virtio_1_0 = virtio_1_0;
+  gcaps_bios_utc = true; (* almost all Linux expect BIOS to be UTC *)
 } in
 
 guestcaps
diff --git a/convert/convert_windows.ml b/convert/convert_windows.ml
index 122d95469d..f6e039be7e 100644
--- a/convert/convert_windows.ml
+++ b/convert/convert_windows.ml
@@ -275,6 +275,7 @@ let convert (g : G.guestfs) _ inspect i_firmware 
block_driver _ static_ips =
   gcaps_arch = Utils.kvm_arch inspect.i_arch;
   gcaps_arch_min_version = 0;
   gcaps_virtio_1_0 = virtio_win_installed.Inject_virtio_win.virtio_1_0;
+  gcaps_bios_utc = true;
 } in
 
 guestcaps
diff --git a/lib/types.ml b/lib/types.ml
index 75c14fd4f6..0f659ebd9a 100644
--- a/lib/types.ml
+++ b/lib/types.ml
@@ -399,6 +399,7 @@ type guestcaps = {
   gcaps_arch : string;
   gcaps_arch_min_version : int;
   gcaps_virtio_1_0 : bool;
+  gcaps_bios_utc : bool;
 }
 and guestcaps_block_type = Virtio_blk | Virtio_SCSI | IDE
 and guestcaps_net_type = Virtio_net | E1000 | RTL8139
@@ -429,6 +430,7 @@ let string_of_guestcaps gcaps =
gcaps_arch = %s\n\
gcaps_arch_min_version = %d\n\
gcaps_virtio_1_0 = %b\n\
+   gcaps_bios_utc = %b\n\
   "
   (string_of_block_type gcaps.gcaps_block_bus)
   (string_of_net_type gcaps.gcaps_net_bus)
@@ -440,6 +442,7 @@ let string_of_guestcaps gcaps =
   gcaps.gcaps_arch
   gcaps.gcaps_arch_min_version
   gcaps.gcaps_virtio_1_0
+  gcaps.gcaps_bios_utc
 
 type target_buses = {
   target_virtio_blk_bus : target_bus_slot array;
-- 
2.41.0

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [EXTERNAL] - Re: Libguestfs desired version installation

[Eric Blake]

On Mon, Sep 25, 2023 at 10:16:10AM +0100, Richard W.M. Jones wrote:
> On Mon, Sep 25, 2023 at 09:09:32AM +, Teja Konapalli wrote:
> > Hi Richard,
> >
> > Yes, you are correct my ask is without upgrading OS version(from 8.2
> > to 9.0) is it possible to upgrade libguestfs alone to different
> > versions.
> 
> No, that is not supported.

Worded differently:

Libguestfs is open source.  Therefore, you have the freedom to install
whatever libguestfs version you want on whatever underlying system you
want - and it may even work the way you want.  However, if you choose
to install a version of libguestfs on a distro that is different than
the version that your distro already provides, then you will be stuck
debugging any issues that arise on your own, and cannot reasonably
expect either your distro or upstream to help you (your distro's
answer will likely be "use the version we already debugged for you").

Upstream support is a limited resource - there are no contractual
guarantees that anyone has to help you, and while it may be low cost
when it works in your favor, you are far more likely to get responses
based on the quality of effort you have put in to reporting problems
accurately, rather than expecting others to do your homework for you.

You mention using RHEL; since you are already paying to use a distro,
you might as well make the most of your support contract.  However,
this is the upstream mailing list, and not the point of contact with
your RHEL support contract.  Then as Rich has already mentioned:

> > 
> > Since you have (still) not described the exact environment you're using, or 
> > even accurately described the problem you're having, I don't know if this 
> > will make any difference.

your distro support contract will likewise expect you to put in more
effort of describing the actual scenario you are trying to solve,
rather than the secondary problem of "can I upgrade libguestfs" that
may or may not address the primary problem that you are facing.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] Fwd: virt-v2v creating image that does not install guest agent on first boot

[Richard W.M. Jones]

On Mon, Sep 25, 2023 at 01:25:06PM +0100, Daniel P. Berrangé wrote:
> On Mon, Sep 25, 2023 at 01:14:40PM +0100, Richard W.M. Jones wrote:
> > On Mon, Sep 25, 2023 at 01:09:15PM +0200, Lee Garrett wrote:
> > > On 23.09.23 19:37, Laszlo Ersek wrote:
> > > >On 9/22/23 16:47, Lee Garrett wrote:
> > > >>On 22.09.23 14:54, Richard W.M. Jones wrote:
> > > >>>On Fri, Sep 22, 2023 at 11:40:03AM +0100, Richard W.M. Jones wrote:
> > > On Thu, Sep 21, 2023 at 07:47:52PM +0200, Lee Garrett wrote:
> > > >On 21.09.23 19:43, Richard W.M. Jones wrote:
> > > >>So this is probably another instance or variation of the timezone
> > > >>formatting problem (of schtasks).  Which version of virt-v2v is 
> > > >>this?
> > > >>I want to check that you have a version with all the latest patches 
> > > >>in
> > > >>this area.
> > > >
> > > >It's 2.2.0-1 from Debian (12) bookworm. I've verified that it
> > > >doesn't have any distro-specific patches.
> > > >
> > > >(https://salsa.debian.org/libvirt-team/virt-v2v/-/tree/debian/master/debian
> > > >would have a patches/series file in this case)
> > > 
> > > The timezone fixes are:
> > > 
> > > commit 597d177567234c3a539098c423649781424eeb6f
> > > Author: Laszlo Ersek 
> > > Date:   Tue Mar 8 15:30:51 2022 +0100
> > > 
> > >   convert_windows: rewrite "configure_qemu_ga" script purely in
> > > PowerShell
> > > 
> > > commit d9dc6c42ae64ba92993dbd9477f003ba73fcfa2f
> > > Author: Richard W.M. Jones 
> > > Date:   Fri Nov 12 08:47:55 2021 +
> > > 
> > >   convert/convert_windows.ml: Handle date formats with dots
> > > instead of /
> > > 
> > > They are all included in >= 2.0
> > > 
> > > I wonder if 597d177567 has a subtle flaw, or if we introduced a bug
> > > somewhere when refactoring this code later.
> > > 
> > > Lee: Do you have a theory about exactly what is wrong with the
> > > schtasks date?  Like what was it supposed to be, assuming it was 120
> > > seconds in the future from boot time, versus what it was set to:
> > > 
> > > >Firstboot-qemu-ga    9/21/2023 4:04:00 PM   Ready
> > > 
> > > Could a date or time field have not been swapped or been corrupted
> > > in some predictable way?
> > > >>>
> > > >>>Or in even simpler terms, what is the time (and timezone) that
> > > >>>this ^^^ machine was booted?
> > > >>
> > > >>I believe I have it figured out.
> > > >>The guest local time is currently 7:08 AM (a few minutes after
> > > >>firstboot/provisioning), pacific daylight time (UTC-7, though Windows
> > > >>displays it as "UTC-08:00"). This is the timezone that the guest comes
> > > >>configured with at first boot. The task is scheduled for 2:01 PM,
> > > >>meaning it's scheduled to run ~7 hours in the future.
> > > >>
> > > >>So it seems like the task was meant to be scheduled for 2:01 PM UTC (=
> > > >>7:01 AM PDT), but for some reason was scheduled for 2:01 PM *local 
> > > >>time*.
> > > >>
> > > >> From what I can see, the host machine time zone is irrelevant (UTC+2).
> > > >>
> > > >>I don't know where the timezone mixup comes from, though. Running
> > > >>`(get-date)` in the powershell at this point correctly returns the local
> > > >>time (7:08 AM). I guess during injection the time is in UTC, and
> > > >>schtasks.exe has no awareness of timezones?
> > > >
> > > >Right, I think there is a timezone disagreement between how we format 
> > > >the timestamp and how schtasks.exe takes it.
> > > >
> > > >What matters here is the /ST (start time) flag.
> > > >
> > > >Today we have (in the common submodule):
> > > >
> > > >   add "$d = (get-date).AddSeconds(120)";
> > > >   add "$dtfinfo = 
> > > > [System.Globalization.DateTimeFormatInfo]::CurrentInfo";
> > > >   add "$sdp = $dtfinfo.ShortDatePattern";
> > > >   add "$sdp = $sdp -replace 'y+', ''";
> > > >   add "$sdp = $sdp -replace 'M+', 'MM'";
> > > >   add "$sdp = $sdp -replace 'd+', 'dd'";
> > > >   add "schtasks.exe /Create /SC ONCE `";
> > > >   add "  /ST $d.ToString('HH:mm') /SD $d.ToString($sdp) `";
> > > >   add "  /RU SYSTEM /TN Firstboot-qemu-ga `";
> > > >   add (sprintf "  /TR \"C:\\%s /forcerestart /qn /l+*vx 
> > > > C:\\%s.log\""
> > > >  msi_path msi_path);
> > > >
> > > >Note that for the /ST option's argument, we only perform the following 
> > > >steps:
> > > >
> > > >   $d = (get-date).AddSeconds(120)
> > > >
> > > >   /ST $d.ToString('HH:mm')
> > > >
> > > >This actually goes back to commit dc66e78fa37d ("windows: delay 
> > > >installation of qemu-ga MSI", 2020-03-10). The timestamp massaging we've 
> > > >since done only targeted the /SD (start date) option, not the start time 
> > > >(/ST) one!
> > > >
> > > >So the problem may be that
> > > >
> > > >   (get-date).AddSeconds(120).ToString('HH:mm')
> > > >
> > > >formats the hour:minute timestamp in 

Re: [Libguestfs] Fwd: virt-v2v creating image that does not install guest agent on first boot

[Daniel P . Berrangé]

On Mon, Sep 25, 2023 at 01:14:40PM +0100, Richard W.M. Jones wrote:
> On Mon, Sep 25, 2023 at 01:09:15PM +0200, Lee Garrett wrote:
> > On 23.09.23 19:37, Laszlo Ersek wrote:
> > >On 9/22/23 16:47, Lee Garrett wrote:
> > >>On 22.09.23 14:54, Richard W.M. Jones wrote:
> > >>>On Fri, Sep 22, 2023 at 11:40:03AM +0100, Richard W.M. Jones wrote:
> > On Thu, Sep 21, 2023 at 07:47:52PM +0200, Lee Garrett wrote:
> > >On 21.09.23 19:43, Richard W.M. Jones wrote:
> > >>So this is probably another instance or variation of the timezone
> > >>formatting problem (of schtasks).  Which version of virt-v2v is this?
> > >>I want to check that you have a version with all the latest patches in
> > >>this area.
> > >
> > >It's 2.2.0-1 from Debian (12) bookworm. I've verified that it
> > >doesn't have any distro-specific patches.
> > >
> > >(https://salsa.debian.org/libvirt-team/virt-v2v/-/tree/debian/master/debian
> > >would have a patches/series file in this case)
> > 
> > The timezone fixes are:
> > 
> > commit 597d177567234c3a539098c423649781424eeb6f
> > Author: Laszlo Ersek 
> > Date:   Tue Mar 8 15:30:51 2022 +0100
> > 
> >   convert_windows: rewrite "configure_qemu_ga" script purely in
> > PowerShell
> > 
> > commit d9dc6c42ae64ba92993dbd9477f003ba73fcfa2f
> > Author: Richard W.M. Jones 
> > Date:   Fri Nov 12 08:47:55 2021 +
> > 
> >   convert/convert_windows.ml: Handle date formats with dots
> > instead of /
> > 
> > They are all included in >= 2.0
> > 
> > I wonder if 597d177567 has a subtle flaw, or if we introduced a bug
> > somewhere when refactoring this code later.
> > 
> > Lee: Do you have a theory about exactly what is wrong with the
> > schtasks date?  Like what was it supposed to be, assuming it was 120
> > seconds in the future from boot time, versus what it was set to:
> > 
> > >Firstboot-qemu-ga    9/21/2023 4:04:00 PM   Ready
> > 
> > Could a date or time field have not been swapped or been corrupted
> > in some predictable way?
> > >>>
> > >>>Or in even simpler terms, what is the time (and timezone) that
> > >>>this ^^^ machine was booted?
> > >>
> > >>I believe I have it figured out.
> > >>The guest local time is currently 7:08 AM (a few minutes after
> > >>firstboot/provisioning), pacific daylight time (UTC-7, though Windows
> > >>displays it as "UTC-08:00"). This is the timezone that the guest comes
> > >>configured with at first boot. The task is scheduled for 2:01 PM,
> > >>meaning it's scheduled to run ~7 hours in the future.
> > >>
> > >>So it seems like the task was meant to be scheduled for 2:01 PM UTC (=
> > >>7:01 AM PDT), but for some reason was scheduled for 2:01 PM *local time*.
> > >>
> > >> From what I can see, the host machine time zone is irrelevant (UTC+2).
> > >>
> > >>I don't know where the timezone mixup comes from, though. Running
> > >>`(get-date)` in the powershell at this point correctly returns the local
> > >>time (7:08 AM). I guess during injection the time is in UTC, and
> > >>schtasks.exe has no awareness of timezones?
> > >
> > >Right, I think there is a timezone disagreement between how we format the 
> > >timestamp and how schtasks.exe takes it.
> > >
> > >What matters here is the /ST (start time) flag.
> > >
> > >Today we have (in the common submodule):
> > >
> > >   add "$d = (get-date).AddSeconds(120)";
> > >   add "$dtfinfo = 
> > > [System.Globalization.DateTimeFormatInfo]::CurrentInfo";
> > >   add "$sdp = $dtfinfo.ShortDatePattern";
> > >   add "$sdp = $sdp -replace 'y+', ''";
> > >   add "$sdp = $sdp -replace 'M+', 'MM'";
> > >   add "$sdp = $sdp -replace 'd+', 'dd'";
> > >   add "schtasks.exe /Create /SC ONCE `";
> > >   add "  /ST $d.ToString('HH:mm') /SD $d.ToString($sdp) `";
> > >   add "  /RU SYSTEM /TN Firstboot-qemu-ga `";
> > >   add (sprintf "  /TR \"C:\\%s /forcerestart /qn /l+*vx C:\\%s.log\""
> > >  msi_path msi_path);
> > >
> > >Note that for the /ST option's argument, we only perform the following 
> > >steps:
> > >
> > >   $d = (get-date).AddSeconds(120)
> > >
> > >   /ST $d.ToString('HH:mm')
> > >
> > >This actually goes back to commit dc66e78fa37d ("windows: delay 
> > >installation of qemu-ga MSI", 2020-03-10). The timestamp massaging we've 
> > >since done only targeted the /SD (start date) option, not the start time 
> > >(/ST) one!
> > >
> > >So the problem may be that
> > >
> > >   (get-date).AddSeconds(120).ToString('HH:mm')
> > >
> > >formats the hour:minute timestamp in UTC (why though?), but the /ST option 
> > >takes hour:minute in local time.
> > >
> > >Interestingly, DateTime objects seem to have a "Kind" property, which may 
> > >be Utc, Local, or Unspec.
> > >
> > >https://learn.microsoft.com/en-us/dotnet/api/system.datetime.kind
> > >
> > >It seems to be used when converting from 

Re: [Libguestfs] Fwd: virt-v2v creating image that does not install guest agent on first boot

[Richard W.M. Jones]

On Mon, Sep 25, 2023 at 01:09:15PM +0200, Lee Garrett wrote:
> On 23.09.23 19:37, Laszlo Ersek wrote:
> >On 9/22/23 16:47, Lee Garrett wrote:
> >>On 22.09.23 14:54, Richard W.M. Jones wrote:
> >>>On Fri, Sep 22, 2023 at 11:40:03AM +0100, Richard W.M. Jones wrote:
> On Thu, Sep 21, 2023 at 07:47:52PM +0200, Lee Garrett wrote:
> >On 21.09.23 19:43, Richard W.M. Jones wrote:
> >>So this is probably another instance or variation of the timezone
> >>formatting problem (of schtasks).  Which version of virt-v2v is this?
> >>I want to check that you have a version with all the latest patches in
> >>this area.
> >
> >It's 2.2.0-1 from Debian (12) bookworm. I've verified that it
> >doesn't have any distro-specific patches.
> >
> >(https://salsa.debian.org/libvirt-team/virt-v2v/-/tree/debian/master/debian
> >would have a patches/series file in this case)
> 
> The timezone fixes are:
> 
> commit 597d177567234c3a539098c423649781424eeb6f
> Author: Laszlo Ersek 
> Date:   Tue Mar 8 15:30:51 2022 +0100
> 
>   convert_windows: rewrite "configure_qemu_ga" script purely in
> PowerShell
> 
> commit d9dc6c42ae64ba92993dbd9477f003ba73fcfa2f
> Author: Richard W.M. Jones 
> Date:   Fri Nov 12 08:47:55 2021 +
> 
>   convert/convert_windows.ml: Handle date formats with dots
> instead of /
> 
> They are all included in >= 2.0
> 
> I wonder if 597d177567 has a subtle flaw, or if we introduced a bug
> somewhere when refactoring this code later.
> 
> Lee: Do you have a theory about exactly what is wrong with the
> schtasks date?  Like what was it supposed to be, assuming it was 120
> seconds in the future from boot time, versus what it was set to:
> 
> >Firstboot-qemu-ga    9/21/2023 4:04:00 PM   Ready
> 
> Could a date or time field have not been swapped or been corrupted
> in some predictable way?
> >>>
> >>>Or in even simpler terms, what is the time (and timezone) that
> >>>this ^^^ machine was booted?
> >>
> >>I believe I have it figured out.
> >>The guest local time is currently 7:08 AM (a few minutes after
> >>firstboot/provisioning), pacific daylight time (UTC-7, though Windows
> >>displays it as "UTC-08:00"). This is the timezone that the guest comes
> >>configured with at first boot. The task is scheduled for 2:01 PM,
> >>meaning it's scheduled to run ~7 hours in the future.
> >>
> >>So it seems like the task was meant to be scheduled for 2:01 PM UTC (=
> >>7:01 AM PDT), but for some reason was scheduled for 2:01 PM *local time*.
> >>
> >> From what I can see, the host machine time zone is irrelevant (UTC+2).
> >>
> >>I don't know where the timezone mixup comes from, though. Running
> >>`(get-date)` in the powershell at this point correctly returns the local
> >>time (7:08 AM). I guess during injection the time is in UTC, and
> >>schtasks.exe has no awareness of timezones?
> >
> >Right, I think there is a timezone disagreement between how we format the 
> >timestamp and how schtasks.exe takes it.
> >
> >What matters here is the /ST (start time) flag.
> >
> >Today we have (in the common submodule):
> >
> >   add "$d = (get-date).AddSeconds(120)";
> >   add "$dtfinfo = 
> > [System.Globalization.DateTimeFormatInfo]::CurrentInfo";
> >   add "$sdp = $dtfinfo.ShortDatePattern";
> >   add "$sdp = $sdp -replace 'y+', ''";
> >   add "$sdp = $sdp -replace 'M+', 'MM'";
> >   add "$sdp = $sdp -replace 'd+', 'dd'";
> >   add "schtasks.exe /Create /SC ONCE `";
> >   add "  /ST $d.ToString('HH:mm') /SD $d.ToString($sdp) `";
> >   add "  /RU SYSTEM /TN Firstboot-qemu-ga `";
> >   add (sprintf "  /TR \"C:\\%s /forcerestart /qn /l+*vx C:\\%s.log\""
> >  msi_path msi_path);
> >
> >Note that for the /ST option's argument, we only perform the following steps:
> >
> >   $d = (get-date).AddSeconds(120)
> >
> >   /ST $d.ToString('HH:mm')
> >
> >This actually goes back to commit dc66e78fa37d ("windows: delay installation 
> >of qemu-ga MSI", 2020-03-10). The timestamp massaging we've since done only 
> >targeted the /SD (start date) option, not the start time (/ST) one!
> >
> >So the problem may be that
> >
> >   (get-date).AddSeconds(120).ToString('HH:mm')
> >
> >formats the hour:minute timestamp in UTC (why though?), but the /ST option 
> >takes hour:minute in local time.
> >
> >Interestingly, DateTime objects seem to have a "Kind" property, which may be 
> >Utc, Local, or Unspec.
> >
> >https://learn.microsoft.com/en-us/dotnet/api/system.datetime.kind
> >
> >It seems to be used when converting from UTC to local or vice versa, and it 
> >probably influences how ToString() behaves too. I thought "get-date" 
> >returned a Local one, and /ST took a local one as well, but perhaps 
> >"get-date" returns a UTC timestamp in this case (when run from the script)?
> 
> I think I have an idea what's 

Re: [Libguestfs] Fwd: virt-v2v creating image that does not install guest agent on first boot

[Lee Garrett]

On 23.09.23 19:37, Laszlo Ersek wrote:

On 9/22/23 16:47, Lee Garrett wrote:

On 22.09.23 14:54, Richard W.M. Jones wrote:

On Fri, Sep 22, 2023 at 11:40:03AM +0100, Richard W.M. Jones wrote:

On Thu, Sep 21, 2023 at 07:47:52PM +0200, Lee Garrett wrote:

On 21.09.23 19:43, Richard W.M. Jones wrote:

So this is probably another instance or variation of the timezone
formatting problem (of schtasks).  Which version of virt-v2v is this?
I want to check that you have a version with all the latest patches in
this area.


It's 2.2.0-1 from Debian (12) bookworm. I've verified that it
doesn't have any distro-specific patches.

(https://salsa.debian.org/libvirt-team/virt-v2v/-/tree/debian/master/debian
would have a patches/series file in this case)


The timezone fixes are:

commit 597d177567234c3a539098c423649781424eeb6f
Author: Laszlo Ersek 
Date:   Tue Mar 8 15:30:51 2022 +0100

  convert_windows: rewrite "configure_qemu_ga" script purely in
PowerShell

commit d9dc6c42ae64ba92993dbd9477f003ba73fcfa2f
Author: Richard W.M. Jones 
Date:   Fri Nov 12 08:47:55 2021 +

  convert/convert_windows.ml: Handle date formats with dots
instead of /

They are all included in >= 2.0

I wonder if 597d177567 has a subtle flaw, or if we introduced a bug
somewhere when refactoring this code later.

Lee: Do you have a theory about exactly what is wrong with the
schtasks date?  Like what was it supposed to be, assuming it was 120
seconds in the future from boot time, versus what it was set to:


Firstboot-qemu-ga    9/21/2023 4:04:00 PM   Ready


Could a date or time field have not been swapped or been corrupted
in some predictable way?


Or in even simpler terms, what is the time (and timezone) that
this ^^^ machine was booted?


I believe I have it figured out.
The guest local time is currently 7:08 AM (a few minutes after
firstboot/provisioning), pacific daylight time (UTC-7, though Windows
displays it as "UTC-08:00"). This is the timezone that the guest comes
configured with at first boot. The task is scheduled for 2:01 PM,
meaning it's scheduled to run ~7 hours in the future.

So it seems like the task was meant to be scheduled for 2:01 PM UTC (=
7:01 AM PDT), but for some reason was scheduled for 2:01 PM *local time*.

 From what I can see, the host machine time zone is irrelevant (UTC+2).

I don't know where the timezone mixup comes from, though. Running
`(get-date)` in the powershell at this point correctly returns the local
time (7:08 AM). I guess during injection the time is in UTC, and
schtasks.exe has no awareness of timezones?


Right, I think there is a timezone disagreement between how we format the 
timestamp and how schtasks.exe takes it.

What matters here is the /ST (start time) flag.

Today we have (in the common submodule):

   add "$d = (get-date).AddSeconds(120)";
   add "$dtfinfo = [System.Globalization.DateTimeFormatInfo]::CurrentInfo";
   add "$sdp = $dtfinfo.ShortDatePattern";
   add "$sdp = $sdp -replace 'y+', ''";
   add "$sdp = $sdp -replace 'M+', 'MM'";
   add "$sdp = $sdp -replace 'd+', 'dd'";
   add "schtasks.exe /Create /SC ONCE `";
   add "  /ST $d.ToString('HH:mm') /SD $d.ToString($sdp) `";
   add "  /RU SYSTEM /TN Firstboot-qemu-ga `";
   add (sprintf "  /TR \"C:\\%s /forcerestart /qn /l+*vx C:\\%s.log\""
  msi_path msi_path);

Note that for the /ST option's argument, we only perform the following steps:

   $d = (get-date).AddSeconds(120)

   /ST $d.ToString('HH:mm')

This actually goes back to commit dc66e78fa37d ("windows: delay installation of 
qemu-ga MSI", 2020-03-10). The timestamp massaging we've since done only targeted 
the /SD (start date) option, not the start time (/ST) one!

So the problem may be that

   (get-date).AddSeconds(120).ToString('HH:mm')

formats the hour:minute timestamp in UTC (why though?), but the /ST option 
takes hour:minute in local time.

Interestingly, DateTime objects seem to have a "Kind" property, which may be 
Utc, Local, or Unspec.

https://learn.microsoft.com/en-us/dotnet/api/system.datetime.kind

It seems to be used when converting from UTC to local or vice versa, and it probably influences how 
ToString() behaves too. I thought "get-date" returned a Local one, and /ST took a local 
one as well, but perhaps "get-date" returns a UTC timestamp in this case (when run from 
the script)?


I think I have an idea what's happening. This is the part of the XML description 
of the guest:




Which sets the time in the guest to UTC during boot.

So these are the steps causing the issue I'm seeing:
1. The machine boots with the clock set to UTC. Windows assumes this is local
   time.
1. The above code snippet gets run in the boot process, scheduling the task to
   run 2 minutes into the future.
2. Within these two minutes, the Windows NTP client runs, which notices a huge
   offset of several hours, and sets the clock back.
3. Since in this case the timezone is UTC-7, the task is 

Re: [Libguestfs] [EXTERNAL] - Re: Libguestfs desired version installation

[Richard W.M. Jones]

On Mon, Sep 25, 2023 at 09:09:32AM +, Teja Konapalli wrote:
> Hi Richard,
>
> Yes, you are correct my ask is without upgrading OS version(from 8.2
> to 9.0) is it possible to upgrade libguestfs alone to different
> versions.

No, that is not supported.

Rich.

> Teja
> 
> -Original Message-
> From: Richard W.M. Jones  
> Sent: Monday, September 25, 2023 2:36 PM
> To: Teja Konapalli 
> Cc: libguestfs@redhat.com; Raja Ram Sharma ; Divyanshu 
> Kumar 
> Subject: [EXTERNAL] - Re: Libguestfs desired version installation
> 
> CAUTION: This email originated from outside of the organization. Do not click 
> links or open attachments unless you recognize the sender and know the 
> content is safe. If you feel that the email is suspicious, please report it 
> using PhishAlarm.
> 
> 
> I read the first email, you don't need to keep sending it.
> 
> If you upgrade RHEL from 8.2 to the latest (8.8 or 8.9) you will get 
> libguestfs 1.44.0.
> 
> If you upgrade to RHEL 9 you will get libguestfs 1.50.1.
> 
> Since you have (still) not described the exact environment you're using, or 
> even accurately described the problem you're having, I don't know if this 
> will make any difference.
> 
> Rich.
> 
> --
> Richard Jones, Virtualization Group, Red Hat 
> https://urldefense.com/v3/__http://people.redhat.com/*rjones__;fg!!Obbck6kTJA!amwb8TmXfcQ2vZeYTfXu4kiNMITCUSbhMBLKj2ZMXMJhxiXtMaNfgWaYXNR9-hQoAQHPNKXHHdDJYB-efg$
> Read my programming and virtualization blog: 
> https://urldefense.com/v3/__http://rwmj.wordpress.com__;!!Obbck6kTJA!amwb8TmXfcQ2vZeYTfXu4kiNMITCUSbhMBLKj2ZMXMJhxiXtMaNfgWaYXNR9-hQoAQHPNKXHHdBA-vNKXQ$
> nbdkit - Flexible, fast NBD server with plugins 
> https://urldefense.com/v3/__https://gitlab.com/nbdkit/nbdkit__;!!Obbck6kTJA!amwb8TmXfcQ2vZeYTfXu4kiNMITCUSbhMBLKj2ZMXMJhxiXtMaNfgWaYXNR9-hQoAQHPNKXHHdBuQVbubQ$

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [EXTERNAL] - Re: Libguestfs desired version installation

[Teja Konapalli]

Hi Richard,

Yes, you are correct my ask is without upgrading OS version(from 8.2 to 9.0) is 
it possible to upgrade libguestfs alone to different versions.
Thanks
Teja

-Original Message-
From: Richard W.M. Jones  
Sent: Monday, September 25, 2023 2:36 PM
To: Teja Konapalli 
Cc: libguestfs@redhat.com; Raja Ram Sharma ; Divyanshu 
Kumar 
Subject: [EXTERNAL] - Re: Libguestfs desired version installation

CAUTION: This email originated from outside of the organization. Do not click 
links or open attachments unless you recognize the sender and know the content 
is safe. If you feel that the email is suspicious, please report it using 
PhishAlarm.


I read the first email, you don't need to keep sending it.

If you upgrade RHEL from 8.2 to the latest (8.8 or 8.9) you will get libguestfs 
1.44.0.

If you upgrade to RHEL 9 you will get libguestfs 1.50.1.

Since you have (still) not described the exact environment you're using, or 
even accurately described the problem you're having, I don't know if this will 
make any difference.

Rich.

--
Richard Jones, Virtualization Group, Red Hat 
https://urldefense.com/v3/__http://people.redhat.com/*rjones__;fg!!Obbck6kTJA!amwb8TmXfcQ2vZeYTfXu4kiNMITCUSbhMBLKj2ZMXMJhxiXtMaNfgWaYXNR9-hQoAQHPNKXHHdDJYB-efg$
Read my programming and virtualization blog: 
https://urldefense.com/v3/__http://rwmj.wordpress.com__;!!Obbck6kTJA!amwb8TmXfcQ2vZeYTfXu4kiNMITCUSbhMBLKj2ZMXMJhxiXtMaNfgWaYXNR9-hQoAQHPNKXHHdBA-vNKXQ$
nbdkit - Flexible, fast NBD server with plugins 
https://urldefense.com/v3/__https://gitlab.com/nbdkit/nbdkit__;!!Obbck6kTJA!amwb8TmXfcQ2vZeYTfXu4kiNMITCUSbhMBLKj2ZMXMJhxiXtMaNfgWaYXNR9-hQoAQHPNKXHHdBuQVbubQ$

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] Libguestfs desired version installation

[Richard W.M. Jones]

I read the first email, you don't need to keep sending it.

If you upgrade RHEL from 8.2 to the latest (8.8 or 8.9) you
will get libguestfs 1.44.0.

If you upgrade to RHEL 9 you will get libguestfs 1.50.1.

Since you have (still) not described the exact environment you're
using, or even accurately described the problem you're having, I don't
know if this will make any difference.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] Libguestfs desired version installation

[Teja Konapalli]

Hi Richard,

Could you please help here .

Thanks
Teja Konapalli

From: Teja Konapalli
Sent: Sunday, September 24, 2023 10:27 PM
To: 'Richard W.M. Jones' 
Cc: 'libguestfs@redhat.com' ; Raja Ram Sharma 
; Divyanshu Kumar 
Subject: RE: Libguestfs desired version installation

Hi Richard,

If possible, to upgrade from lower version to higher version, Could you please 
help us with the upgrade steps.

Regards
Teja K

From: Teja Konapalli
Sent: Sunday, September 24, 2023 10:11 AM
To: Richard W.M. Jones mailto:rjo...@redhat.com>>
Cc: libguestfs@redhat.com; Raja Ram Sharma 
mailto:rshar...@opentext.com>>; Divyanshu Kumar 
mailto:dkum...@opentext.com>>
Subject: Libguestfs desired version installation

Hi Richard ,

Happy weekend.

I had one question regarding desired version installation of libguestfs, 
suppose if had RHEL 8.0 default libguestfs is 1.38.0 and I want to install 
1.50.0 in same machine is it possible to install?

Thanks
Teja K

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs