[RFC PATCH 0/1] Implement entropy leak reporting for virtio-rng

[RFC PATCH 0/1] Implement entropy leak reporting for virtio-rng

[Babis Chalios]

This patchset implements the entropy leak reporting feature proposal [1]
for virtio-rng devices.

Entropy leaking (as defined in the specification proposal) typically
happens when we take a snapshot of a VM or while we resume a VM from a
snapshot. In these cases, we want to let the guest know so that it can
reset state that needs to be uniqueue, for example.

This feature is offering functionality similar to what VMGENID does.
However, it allows to build mechanisms on the guest side to notify
user-space applications, like VMGENID for userspace and additionally for
kernel.

The new specification describes two request types that the guest might
place in the queues for the device to perform, a fill-on-leak request
where the device needs to fill with random bytes a buffer and a
copy-on-leak request where the device needs to perform a copy between
two guest-provided buffers. We currently trigger the handling of guest
requests when saving the VM state and when loading a VM from a snapshot
file.

This is an RFC, since the corresponding specification changes have not
yet been merged. It also aims to allow testing a respective patch-set
implementing the feature in the Linux front-end driver[2].

However, I would like to ask the community's opinion regarding the
handling of the fill-on-leak requests. Essentially, these requests are
very similar to the normal virtio-rng entropy requests, with the catch
that we should complete these requests before resuming the VM, so that
we avoid race-conditions in notifying the guest about entropy leak
events. This means that we cannot rely on the RngBackend's API, which is
asynchronous. At the moment, I have handled that using getrandom(), but
I would like a solution which doesn't work only with (relatively new)
Linux hosts. I am inclined to solve that by extending the RngBackend API
with a synchronous call to request for random bytes and I'd like to hear
opinion's on this approach.

[1] https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg09016.html
[2] https://lore.kernel.org/lkml/20230131145543.86369-1-bchalios@amazon.es/

Babis Chalios (1):
  virtio-rng: implement entropy leak feature

 hw/virtio/virtio-rng.c                      | 170 +++++++++++++++++++-
 include/hw/virtio/virtio-rng.h              |   9 +-
 include/standard-headers/linux/virtio_rng.h |   3 +
 3 files changed, 179 insertions(+), 3 deletions(-)

-- 
2.39.2

[RFC PATCH 1/1] virtio-rng: implement entropy leak feature

[Babis Chalios]

Entropy leak reporting is a virtio-rng proposed feature [1], which
allows a virtio-rng device to report events during which entropy has
decreased, for example when a VM gets snapshotted or resumed from a
snapshot.

Guests can request from the virtio-rng device to perform two types of
events upon such events: 1. fill a buffer with random bytes or 2. copy
the contents of one buffer to another. A guest adds requests to "leak
queues" which the device will handle only when an "entropy leak" event
happens.

This patch extends the virtio-rng device with the pair of leak queues
defined in the specification proposal and implements the two types of
requests. At the moment, it triggers handling of requests during
post_save() and post_load() hooks.

In the current version, "fill-on-leak" request makes use of getrandom()
to get random bytes. However, a proper implementation should probably
consider extending the RngBackend API to include a synchronous call.

[1] https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg09016.html

Signed-off-by: Babis Chalios <bchalios@amazon.es>
---
 hw/virtio/virtio-rng.c                      | 170 +++++++++++++++++++-
 include/hw/virtio/virtio-rng.h              |   9 +-
 include/standard-headers/linux/virtio_rng.h |   3 +
 3 files changed, 179 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index 7e12fc03bf..def1b1d994 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -9,6 +9,7 @@
  * top-level directory.
  */
 
+#include <sys/random.h>
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu/iov.h"
@@ -20,7 +21,11 @@
 #include "sysemu/rng.h"
 #include "sysemu/runstate.h"
 #include "qom/object_interfaces.h"
+#include "migration/misc.h"
 #include "trace.h"
+#include <stdint.h>
+
+#define VIRTIO_RNG_VM_VERSION  1
 
 static bool is_guest_ready(VirtIORNG *vrng)
 {
@@ -43,6 +48,112 @@ static size_t get_request_size(VirtQueue *vq, unsigned quota)
 
 static void virtio_rng_process(VirtIORNG *vrng);
 
+static VirtQueue *get_active_leak_queue(VirtIORNG *vrng)
+{
+    size_t queue = vrng->active_leak_queue;
+    return vrng->leakq[queue];
+}
+
+static size_t swap_active_leak_queue(VirtIORNG *vrng)
+{
+    size_t old_active = vrng->active_leak_queue;
+    vrng->active_leak_queue = (old_active + 1) % 2;
+    return old_active;
+}
+
+static VirtQueue *get_signaled_leak_queue(VirtIORNG *vrng)
+{
+    int32_t signaled_leak_queue = vrng->signaled_leak_queue;
+
+    if (signaled_leak_queue == -1) {
+        return NULL;
+    }
+
+    return vrng->leakq[signaled_leak_queue];
+}
+
+static size_t handle_fill_on_leak_command(VirtIORNG *vrng, VirtQueue *vq,
+                                          VirtQueueElement *elem)
+{
+    size_t bytes = iov_size(elem->in_sg, elem->in_num);
+    uint8_t *buffer = g_new0(uint8_t, bytes);
+
+    /*
+     * Probably, the correct thing to do is add a synchronous
+     * API call to RngBackend and use it here.
+     */
+    if (getrandom(buffer, bytes, 0) != bytes) {
+        fprintf(stderr, "qemu-virtio-rng: could not get random bytes");
+        return 0;
+    }
+
+    iov_from_buf(elem->in_sg, elem->in_num, 0, buffer, bytes);
+
+    return bytes;
+}
+
+static size_t handle_copy_on_leak_command(VirtIORNG *vrng, VirtQueue *vq,
+                                          VirtQueueElement *elem)
+{
+    size_t out_size, in_size, offset = 0;
+
+    out_size = iov_size(elem->out_sg, elem->out_num);
+    in_size = iov_size(elem->in_sg, elem->in_num);
+
+    if (out_size != in_size) {
+        return 0;
+    }
+
+    for (int i = 0; i < elem->out_num; ++i) {
+        struct iovec *iov = &elem->out_sg[i];
+        offset += iov_from_buf(elem->in_sg, elem->in_num, offset, iov->iov_base,
+                               iov->iov_len);
+    }
+
+    return offset;
+}
+
+static void virtio_rng_process_leak(VirtIORNG *vrng, VirtQueue *vq)
+{
+    VirtQueueElement *elem;
+    VirtIODevice *vdev = VIRTIO_DEVICE(vrng);
+    size_t len;
+
+    if (!runstate_check(RUN_STATE_RUNNING)) {
+        return;
+    }
+
+    while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) {
+        /*
+         * If we have a write buffer this is a copy-on-leak command
+         * otherwise a fill-on-leak command
+         */
+        if (elem->out_num) {
+            len = handle_copy_on_leak_command(vrng, vq, elem);
+        } else {
+            len = handle_fill_on_leak_command(vrng, vq, elem);
+        }
+
+        virtqueue_push(vq, elem, len);
+        g_free(elem);
+    }
+    virtio_notify(vdev, vq);
+}
+
+static int signal_entropy_leak(VirtIORNG *vrng)
+{
+    VirtQueue *activeq = get_active_leak_queue(vrng);
+
+    /*
+     * Process all the buffers in the active leak queue
+     * and then swap active leak queues.
+     */
+    virtio_rng_process_leak(vrng, activeq);
+    vrng->signaled_leak_queue = swap_active_leak_queue(vrng);
+
+    return 0;
+}
+
 /* Send data from a char device over to the guest */
 static void chr_read(void *opaque, const void *buf, size_t size)
 {
@@ -128,9 +239,29 @@ static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
     virtio_rng_process(vrng);
 }
 
+static void handle_leakq(VirtIODevice *vdev, VirtQueue *vq)
+{
+    VirtIORNG *vrng = VIRTIO_RNG(vdev);
+    VirtQueue *signaled_queue = get_signaled_leak_queue(vrng);
+
+    if (!is_guest_ready(vrng)) {
+        return;
+    }
+
+    /*
+     * If we received a request on an already signalled leak queue
+     * we need to handle it immediately, otherwise we leave the buffer(s)
+     * in the virtqueue and we will handle them once an entropy leak event
+     * occurs.
+     */
+    if (vq == signaled_queue) {
+        virtio_rng_process_leak(vrng, vq);
+    }
+}
+
 static uint64_t get_features(VirtIODevice *vdev, uint64_t f, Error **errp)
 {
-    return f;
+    return f | (1 << VIRTIO_RNG_F_LEAK);
 }
 
 static void virtio_rng_vm_state_change(void *opaque, bool running,
@@ -218,11 +349,14 @@ static void virtio_rng_device_realize(DeviceState *dev, Error **errp)
     virtio_init(vdev, VIRTIO_ID_RNG, 0);
 
     vrng->vq = virtio_add_queue(vdev, 8, handle_input);
+    vrng->leakq[0] = virtio_add_queue(vdev, 8, handle_leakq);
+    vrng->leakq[1] = virtio_add_queue(vdev, 8, handle_leakq);
+    vrng->active_leak_queue = 0;
+    vrng->signaled_leak_queue = -1;
     vrng->quota_remaining = vrng->conf.max_bytes;
     vrng->rate_limit_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
                                                check_rate_limit, vrng);
     vrng->activate_timer = true;
-
     vrng->vmstate = qemu_add_vm_change_state_handler(virtio_rng_vm_state_change,
                                                      vrng);
 }
@@ -235,9 +369,40 @@ static void virtio_rng_device_unrealize(DeviceState *dev)
     qemu_del_vm_change_state_handler(vrng->vmstate);
     timer_free(vrng->rate_limit_timer);
     virtio_del_queue(vdev, 0);
+    virtio_del_queue(vdev, 1);
+    virtio_del_queue(vdev, 2);
     virtio_cleanup(vdev);
 }
 
+/*
+ * After saving the VM state or loading a VM from a snapshot,
+ * we need to signal the guest for a leak event
+ */
+static int virtio_rng_post_save_device(void *opaque)
+{
+    VirtIORNG *vrng = opaque;
+    return signal_entropy_leak(vrng);
+}
+
+static int virtio_rng_post_load_device(void *opaque, int version_id)
+{
+    VirtIORNG *vrng = opaque;
+    return signal_entropy_leak(vrng);
+}
+
+static const VMStateDescription vmstate_virtio_rng_device = {
+    .name = "virtio-rng-device",
+    .version_id = VIRTIO_RNG_VM_VERSION,
+    .minimum_version_id = VIRTIO_RNG_VM_VERSION,
+    .post_save = virtio_rng_post_save_device,
+    .post_load = virtio_rng_post_load_device,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(active_leak_queue, VirtIORNG),
+        VMSTATE_INT32(signaled_leak_queue, VirtIORNG),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_virtio_rng = {
     .name = "virtio-rng",
     .minimum_version_id = 1,
@@ -272,6 +437,7 @@ static void virtio_rng_class_init(ObjectClass *klass, void *data)
     vdc->unrealize = virtio_rng_device_unrealize;
     vdc->get_features = get_features;
     vdc->set_status = virtio_rng_set_status;
+    vdc->vmsd = &vmstate_virtio_rng_device;
 }
 
 static const TypeInfo virtio_rng_info = {
diff --git a/include/hw/virtio/virtio-rng.h b/include/hw/virtio/virtio-rng.h
index 82734255d9..0d492c0ac7 100644
--- a/include/hw/virtio/virtio-rng.h
+++ b/include/hw/virtio/virtio-rng.h
@@ -31,8 +31,15 @@ struct VirtIORNGConf {
 struct VirtIORNG {
     VirtIODevice parent_obj;
 
-    /* Only one vq - guest puts buffer(s) on it when it needs entropy */
+    /*
+     * One vq for entropy requests and a pair of vqs for the reporting entropy
+     * leak events
+     */
     VirtQueue *vq;
+    VirtQueue *leakq[2];
+
+    uint32_t active_leak_queue;
+    int32_t signaled_leak_queue;
 
     VirtIORNGConf conf;
 
diff --git a/include/standard-headers/linux/virtio_rng.h b/include/standard-headers/linux/virtio_rng.h
index 60fc798bde..ffe0c6841d 100644
--- a/include/standard-headers/linux/virtio_rng.h
+++ b/include/standard-headers/linux/virtio_rng.h
@@ -5,4 +5,7 @@
 #include "standard-headers/linux/virtio_ids.h"
 #include "standard-headers/linux/virtio_config.h"
 
+/* Feature bits */
+#define VIRTIO_RNG_F_LEAK 0
+
 #endif /* _LINUX_VIRTIO_RNG_H */
-- 
2.39.2

Re: [RFC PATCH 0/1] Implement entropy leak reporting for virtio-rng

[Jason A. Donenfeld]

Hi Babis,

Why are you resending this? As I mentioned before, I'm going to move
forward in implementing this feature in a way that actually works with
the RNG. I'll use your RFC patch as a base, but I think beyond that, I
can take it from here.

Thanks,
Jason

Re: [RFC PATCH 0/1] Implement entropy leak reporting for virtio-rng

[Jason A. Donenfeld]

On Mon, Apr 3, 2023 at 4:15 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Hi Babis,
>
> Why are you resending this? As I mentioned before, I'm going to move
> forward in implementing this feature in a way that actually works with
> the RNG. I'll use your RFC patch as a base, but I think beyond that, I
> can take it from here.

Grrr, sorry! This is for QEMU! I understand.

(Kernel ends from me are forthcoming.)

Jason

Re: [RFC PATCH 0/1] Implement entropy leak reporting for virtio-rng

[bchalios]

On 4/3/23 4:16 PM, "Jason A. Donenfeld" <Jason@zx2c4.com> wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On Mon, Apr 3, 2023 at 4:15 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > Hi Babis,
> >
> > Why are you resending this? As I mentioned before, I'm going to move
> > forward in implementing this feature in a way that actually works with
> > the RNG. I'll use your RFC patch as a base, but I think beyond that, I
> > can take it from here.
> 
> Grrr, sorry! This is for QEMU! I understand.
> 
> (Kernel ends from me are forthcoming.)
> 
> Jason
> 

Hey Jason,

Good to hear from you. Yeap, I thought it would be nice to be able to test this using QEMU (apart from Firecracker).

Cheers,
Babis 

Re: [RFC PATCH 0/1] Implement entropy leak reporting for virtio-rng

[Amit Shah]

Hey Babis,

On Mon, 2023-04-03 at 12:52 +0200, Babis Chalios wrote:
> This patchset implements the entropy leak reporting feature proposal [1]
> for virtio-rng devices.
> 
> Entropy leaking (as defined in the specification proposal) typically
> happens when we take a snapshot of a VM or while we resume a VM from a
> snapshot. In these cases, we want to let the guest know so that it can
> reset state that needs to be uniqueue, for example.
> 
> This feature is offering functionality similar to what VMGENID does.
> However, it allows to build mechanisms on the guest side to notify
> user-space applications, like VMGENID for userspace and additionally for
> kernel.
> 
> The new specification describes two request types that the guest might
> place in the queues for the device to perform, a fill-on-leak request
> where the device needs to fill with random bytes a buffer and a
> copy-on-leak request where the device needs to perform a copy between
> two guest-provided buffers. We currently trigger the handling of guest
> requests when saving the VM state and when loading a VM from a snapshot
> file.
> 
> This is an RFC, since the corresponding specification changes have not
> yet been merged. It also aims to allow testing a respective patch-set
> implementing the feature in the Linux front-end driver[2].
> 
> However, I would like to ask the community's opinion regarding the
> handling of the fill-on-leak requests. Essentially, these requests are
> very similar to the normal virtio-rng entropy requests, with the catch
> that we should complete these requests before resuming the VM, so that
> we avoid race-conditions in notifying the guest about entropy leak
> events. This means that we cannot rely on the RngBackend's API, which is
> asynchronous. At the moment, I have handled that using getrandom(), but
> I would like a solution which doesn't work only with (relatively new)
> Linux hosts. I am inclined to solve that by extending the RngBackend API
> with a synchronous call to request for random bytes and I'd like to hear
> opinion's on this approach.

The patch looks OK - I suggest you add a new sync call that also probes
for the availability of getrandom().  If that doesn't exist, your new
code should figure out a way to deal with the lack of that call.

On older Linux or non-Linux, there are other ways of getting random
numbers, so I expect that sync backend you introduce get more capable.

		Amit

Re: [RFC PATCH 0/1] Implement entropy leak reporting for virtio-rng

[Jason A. Donenfeld]

On Tue, Apr 11, 2023 at 6:19 PM Amit Shah <amit@infradead.org> wrote:
>
> Hey Babis,
>
> On Mon, 2023-04-03 at 12:52 +0200, Babis Chalios wrote:
> > This patchset implements the entropy leak reporting feature proposal [1]
> > for virtio-rng devices.
> >
> > Entropy leaking (as defined in the specification proposal) typically
> > happens when we take a snapshot of a VM or while we resume a VM from a
> > snapshot. In these cases, we want to let the guest know so that it can
> > reset state that needs to be uniqueue, for example.
> >
> > This feature is offering functionality similar to what VMGENID does.
> > However, it allows to build mechanisms on the guest side to notify
> > user-space applications, like VMGENID for userspace and additionally for
> > kernel.
> >
> > The new specification describes two request types that the guest might
> > place in the queues for the device to perform, a fill-on-leak request
> > where the device needs to fill with random bytes a buffer and a
> > copy-on-leak request where the device needs to perform a copy between
> > two guest-provided buffers. We currently trigger the handling of guest
> > requests when saving the VM state and when loading a VM from a snapshot
> > file.
> >
> > This is an RFC, since the corresponding specification changes have not
> > yet been merged. It also aims to allow testing a respective patch-set
> > implementing the feature in the Linux front-end driver[2].
> >
> > However, I would like to ask the community's opinion regarding the
> > handling of the fill-on-leak requests. Essentially, these requests are
> > very similar to the normal virtio-rng entropy requests, with the catch
> > that we should complete these requests before resuming the VM, so that
> > we avoid race-conditions in notifying the guest about entropy leak
> > events. This means that we cannot rely on the RngBackend's API, which is
> > asynchronous. At the moment, I have handled that using getrandom(), but
> > I would like a solution which doesn't work only with (relatively new)
> > Linux hosts. I am inclined to solve that by extending the RngBackend API
> > with a synchronous call to request for random bytes and I'd like to hear
> > opinion's on this approach.
>
> The patch looks OK - I suggest you add a new sync call that also probes
> for the availability of getrandom().

qemu_guest_getrandom_nofail?

Re: [RFC PATCH 0/1] Implement entropy leak reporting for virtio-rng

[Babis Chalios]

On 11/4/23 18:20, Jason A. Donenfeld wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On Tue, Apr 11, 2023 at 6:19 PM Amit Shah <amit@infradead.org> wrote:
>> Hey Babis,
>>
>> On Mon, 2023-04-03 at 12:52 +0200, Babis Chalios wrote:
>>> This patchset implements the entropy leak reporting feature proposal [1]
>>> for virtio-rng devices.
>>>
>>> Entropy leaking (as defined in the specification proposal) typically
>>> happens when we take a snapshot of a VM or while we resume a VM from a
>>> snapshot. In these cases, we want to let the guest know so that it can
>>> reset state that needs to be uniqueue, for example.
>>>
>>> This feature is offering functionality similar to what VMGENID does.
>>> However, it allows to build mechanisms on the guest side to notify
>>> user-space applications, like VMGENID for userspace and additionally for
>>> kernel.
>>>
>>> The new specification describes two request types that the guest might
>>> place in the queues for the device to perform, a fill-on-leak request
>>> where the device needs to fill with random bytes a buffer and a
>>> copy-on-leak request where the device needs to perform a copy between
>>> two guest-provided buffers. We currently trigger the handling of guest
>>> requests when saving the VM state and when loading a VM from a snapshot
>>> file.
>>>
>>> This is an RFC, since the corresponding specification changes have not
>>> yet been merged. It also aims to allow testing a respective patch-set
>>> implementing the feature in the Linux front-end driver[2].
>>>
>>> However, I would like to ask the community's opinion regarding the
>>> handling of the fill-on-leak requests. Essentially, these requests are
>>> very similar to the normal virtio-rng entropy requests, with the catch
>>> that we should complete these requests before resuming the VM, so that
>>> we avoid race-conditions in notifying the guest about entropy leak
>>> events. This means that we cannot rely on the RngBackend's API, which is
>>> asynchronous. At the moment, I have handled that using getrandom(), but
>>> I would like a solution which doesn't work only with (relatively new)
>>> Linux hosts. I am inclined to solve that by extending the RngBackend API
>>> with a synchronous call to request for random bytes and I'd like to hear
>>> opinion's on this approach.
>> The patch looks OK - I suggest you add a new sync call that also probes
>> for the availability of getrandom().
> qemu_guest_getrandom_nofail?

That should work, I think. Any objections to this Amit?

Cheers,
Babis

Re: [RFC PATCH 0/1] Implement entropy leak reporting for virtio-rng

[Amit Shah]

On Thu, 2023-04-13 at 15:36 +0200, Babis Chalios wrote:
> 
> On 11/4/23 18:20, Jason A. Donenfeld wrote:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > 
> > 
> > 
> > On Tue, Apr 11, 2023 at 6:19 PM Amit Shah <amit@infradead.org> wrote:
> > > Hey Babis,
> > > 
> > > On Mon, 2023-04-03 at 12:52 +0200, Babis Chalios wrote:
> > > > This patchset implements the entropy leak reporting feature proposal [1]
> > > > for virtio-rng devices.
> > > > 
> > > > Entropy leaking (as defined in the specification proposal) typically
> > > > happens when we take a snapshot of a VM or while we resume a VM from a
> > > > snapshot. In these cases, we want to let the guest know so that it can
> > > > reset state that needs to be uniqueue, for example.
> > > > 
> > > > This feature is offering functionality similar to what VMGENID does.
> > > > However, it allows to build mechanisms on the guest side to notify
> > > > user-space applications, like VMGENID for userspace and additionally for
> > > > kernel.
> > > > 
> > > > The new specification describes two request types that the guest might
> > > > place in the queues for the device to perform, a fill-on-leak request
> > > > where the device needs to fill with random bytes a buffer and a
> > > > copy-on-leak request where the device needs to perform a copy between
> > > > two guest-provided buffers. We currently trigger the handling of guest
> > > > requests when saving the VM state and when loading a VM from a snapshot
> > > > file.
> > > > 
> > > > This is an RFC, since the corresponding specification changes have not
> > > > yet been merged. It also aims to allow testing a respective patch-set
> > > > implementing the feature in the Linux front-end driver[2].
> > > > 
> > > > However, I would like to ask the community's opinion regarding the
> > > > handling of the fill-on-leak requests. Essentially, these requests are
> > > > very similar to the normal virtio-rng entropy requests, with the catch
> > > > that we should complete these requests before resuming the VM, so that
> > > > we avoid race-conditions in notifying the guest about entropy leak
> > > > events. This means that we cannot rely on the RngBackend's API, which is
> > > > asynchronous. At the moment, I have handled that using getrandom(), but
> > > > I would like a solution which doesn't work only with (relatively new)
> > > > Linux hosts. I am inclined to solve that by extending the RngBackend API
> > > > with a synchronous call to request for random bytes and I'd like to hear
> > > > opinion's on this approach.
> > > The patch looks OK - I suggest you add a new sync call that also probes
> > > for the availability of getrandom().
> > qemu_guest_getrandom_nofail?
> 
> That should work, I think. Any objections to this Amit?

That's one way to do it - and is fine too - but still needs probing
before calling in that function to ensure it's not going to fail.

		Amit