6:37 a.m.
Patch 1 seems uncontroversial.
Patch 2 is tricky. This is related to the following RFE:
https://github.com/libguestfs/libguestfs/issues/80
I initially believed that the reporter wanted to just associate some
general data per drive, and wanted to reuse the (unused) name field
for this. That's what this patch implements.
However I since believe that he's in fact trying to just get the drive
name of the drive that was just added. There's no need for any API
for that, you can just assume drives are called (from the guestfs API
point of view) /dev/sda, /dev/sdb etc.
Adding patch 2 probably confuses things, since I guess most people
would assume that an API called guestfs_get_drive_name would return
the drive name (/dev/sda) not some string that you have to add.
Rich.
6:37 a.m.
New subject: [PATCH 1/2] api: Note that drive "name" field is no longer used
Before commit 3a00c4d179 ("Remove inspection from the C library and
switch to daemon/OCaml implementation") in 2017 the name parameter
passed to add_drive was used by inspection to override the device name
that is determined from fstab. None of our tools ever actually used
this parameter, and when the inspection code was moved inside the
daemon we stopped using this hint field at all.
So it's no longer used, and likely hasn't been used ever. Therefore
document that the field does nothing.
---
generator/actions_core.ml | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/generator/actions_core.ml b/generator/actions_core.ml
index 23a2eabef8..ce9ee39cca 100644
--- a/generator/actions_core.ml
+++ b/generator/actions_core.ml
@@ -263,9 +263,8 @@ deprecated C<guestfs_add_drive_with_if> call (q.v.)
=item C<name>
-The name the drive had in the original guest, e.g. F</dev/sdb>.
-This is used as a hint to the guest inspection process if
-it is available.
+This field used to be passed as a hint for guest inspection, but
+it is no longer used.
=item C<label>
--
2.35.1
5:01 a.m.
New subject: [PATCH 1/2] api: Note that drive "name" field is no longer used
On 04/28/22 13:37, Richard W.M. Jones wrote:
Before commit 3a00c4d179 ("Remove inspection from the C library
and
switch to daemon/OCaml implementation") in 2017 the name parameter
passed to add_drive was used by inspection to override the device name
that is determined from fstab. None of our tools ever actually used
this parameter, and when the inspection code was moved inside the
daemon we stopped using this hint field at all.
So it's no longer used, and likely hasn't been used ever. Therefore
document that the field does nothing.
---
generator/actions_core.ml | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/generator/actions_core.ml b/generator/actions_core.ml
index 23a2eabef8..ce9ee39cca 100644
--- a/generator/actions_core.ml
+++ b/generator/actions_core.ml
@@ -263,9 +263,8 @@ deprecated C<guestfs_add_drive_with_if> call (q.v.)
=item C<name>
-The name the drive had in the original guest, e.g. F</dev/sdb>.
-This is used as a hint to the guest inspection process if
-it is available.
+This field used to be passed as a hint for guest inspection, but
+it is no longer used.
=item C<label>
Reviewed-by: Laszlo Ersek <lersek(a)redhat.com>
5:24 a.m.
New subject: [PATCH 1/2] api: Note that drive "name" field is no longer used
On Fri, Apr 29, 2022 at 12:01:23PM +0200, Laszlo Ersek wrote:
On 04/28/22 13:37, Richard W.M. Jones wrote:
> Before commit 3a00c4d179 ("Remove inspection from the C library and
> switch to daemon/OCaml implementation") in 2017 the name parameter
> passed to add_drive was used by inspection to override the device name
> that is determined from fstab. None of our tools ever actually used
> this parameter, and when the inspection code was moved inside the
> daemon we stopped using this hint field at all.
>
> So it's no longer used, and likely hasn't been used ever. Therefore
> document that the field does nothing.
> ---
> generator/actions_core.ml | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/generator/actions_core.ml b/generator/actions_core.ml
> index 23a2eabef8..ce9ee39cca 100644
> --- a/generator/actions_core.ml
> +++ b/generator/actions_core.ml
> @@ -263,9 +263,8 @@ deprecated C<guestfs_add_drive_with_if> call (q.v.)
>
> =item C<name>
>
> -The name the drive had in the original guest, e.g. F</dev/sdb>.
> -This is used as a hint to the guest inspection process if
> -it is available.
> +This field used to be passed as a hint for guest inspection, but
> +it is no longer used.
>
> =item C<label>
>
>
Reviewed-by: Laszlo Ersek <lersek(a)redhat.com>
I pushed this one, commit b4081d0275, since it's just a simple
doc change and not really related to the other patch.
Thanks,
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
6:37 a.m.
New subject: [PATCH 2/2] lib: Add API for reading the "name" field for a drive
It is likely this field was never used. Repurpose it as a general
scratchpad for callers to use to save some per-drive data.
Fixes: https://github.com/libguestfs/libguestfs/issues/80
---
generator/actions_core.ml | 30 ++++++++++++++++++++++++++++--
lib/drives.c | 20 ++++++++++++++++++++
tests/c-api/tests-main.c | 4 +++-
3 files changed, 51 insertions(+), 3 deletions(-)
diff --git a/generator/actions_core.ml b/generator/actions_core.ml
index ce9ee39cca..44b0c22f29 100644
--- a/generator/actions_core.ml
+++ b/generator/actions_core.ml
@@ -263,8 +263,9 @@ deprecated C<guestfs_add_drive_with_if> call (q.v.)
=item C<name>
-This field used to be passed as a hint for guest inspection, but
-it is no longer used.
+This is an arbitrary string. If you pass in a string here, you
+can read it back by calling C<guestfs_get_drive_name>. It is
+not used by libguestfs itself.
=item C<label>
@@ -1507,6 +1508,31 @@ This call is used by L<virt-rescue(1)> to write directly to
appliance console (for passing through keystrokes). It should
not normally be used by other libguestfs users." };
+ { defaults with
+ name = "get_drive_name"; added = (1, 49, 1);
+ style = RString (RPlainString, "name"), [Int "index"], [];
+ blocking = false;
+ tests = [
+ InitEmpty, Always, TestResult (
+ [["get_drive_name"; "0"]], "STREQ (ret,
\"test\")"), [];
+ InitEmpty, Always, TestLastFail (
+ [["get_drive_name"; "1"]]), [];
+ InitEmpty, Always, TestLastFail (
+ [["get_drive_name"; "99"]]), [];
+ ];
+ shortdesc = "get the drive name";
+ longdesc = "\
+Return the optional C<name> field passed to C<guestfs_add_drive_opts>
+when n'th drive C<index> was added.
+
+If there is no C<name> field associated with the drive
+then this function returns an error, with error code C<ESRCH>.
+
+If you only have a device name and not an index, you can call
+C<guestfs_device_index>. If what you want instead is the
+drive name as it appears to libguestfs, there is no function
+for that, instead use C<\"/dev/sda\">, C<\"/dev/sdb\">
etc." };
+
]
let daemon_functions = [
diff --git a/lib/drives.c b/lib/drives.c
index fd95308d2d..0da730c734 100644
--- a/lib/drives.c
+++ b/lib/drives.c
@@ -31,6 +31,7 @@
#include <netdb.h>
#include <arpa/inet.h>
#include <assert.h>
+#include <errno.h>
#include <libintl.h>
#include "c-ctype.h"
@@ -594,6 +595,25 @@ guestfs_int_free_drives (guestfs_h *g)
g->nr_drives = 0;
}
+char *
+guestfs_impl_get_drive_name (guestfs_h *g, int i)
+{
+ const char *name;
+
+ if (i < 0 || i > g->nr_drives) {
+ guestfs_int_error_errno (g, EINVAL, _("drive index out of range"));
+ return NULL;
+ }
+
+ name = g->drives[i]->name;
+ if (!name) {
+ guestfs_int_error_errno (g, ESRCH, _("drive does not have a name"));
+ return NULL;
+ }
+
+ return safe_strdup (g, name);
+}
+
/**
* Check string parameter matches regular expression
* C<^[-_[:alnum:]]+$> (in C locale).
diff --git a/tests/c-api/tests-main.c b/tests/c-api/tests-main.c
index 9e01e4a3dc..a0aad1735d 100644
--- a/tests/c-api/tests-main.c
+++ b/tests/c-api/tests-main.c
@@ -431,7 +431,9 @@ create_handle (void)
exit (EXIT_FAILURE);
}
- if (guestfs_add_drive_scratch (g, INT64_C(2)*1024*1024*1024, -1) == -1) {
+ if (guestfs_add_drive_scratch (g, INT64_C(2)*1024*1024*1024,
+ GUESTFS_ADD_DRIVE_SCRATCH_NAME, "test",
+ -1) == -1) {
printf ("FAIL: guestfs_add_drive_scratch\n");
exit (EXIT_FAILURE);
}
--
2.35.1
5:01 a.m.
New subject: [PATCH 2/2] lib: Add API for reading the "name" field for a drive
On 04/28/22 13:37, Richard W.M. Jones wrote:
It is likely this field was never used. Repurpose it as a general
scratchpad for callers to use to save some per-drive data.
Fixes: https://github.com/libguestfs/libguestfs/issues/80
---
generator/actions_core.ml | 30 ++++++++++++++++++++++++++++--
lib/drives.c | 20 ++++++++++++++++++++
tests/c-api/tests-main.c | 4 +++-
3 files changed, 51 insertions(+), 3 deletions(-)
diff --git a/generator/actions_core.ml b/generator/actions_core.ml
index ce9ee39cca..44b0c22f29 100644
--- a/generator/actions_core.ml
+++ b/generator/actions_core.ml
@@ -263,8 +263,9 @@ deprecated C<guestfs_add_drive_with_if> call (q.v.)
=item C<name>
-This field used to be passed as a hint for guest inspection, but
-it is no longer used.
+This is an arbitrary string. If you pass in a string here, you
+can read it back by calling C<guestfs_get_drive_name>. It is
+not used by libguestfs itself.
=item C<label>
@@ -1507,6 +1508,31 @@ This call is used by L<virt-rescue(1)> to write directly to
appliance console (for passing through keystrokes). It should
not normally be used by other libguestfs users." };
+ { defaults with
+ name = "get_drive_name"; added = (1, 49, 1);
+ style = RString (RPlainString, "name"), [Int "index"], [];
+ blocking = false;
+ tests = [
+ InitEmpty, Always, TestResult (
+ [["get_drive_name"; "0"]], "STREQ (ret,
\"test\")"), [];
+ InitEmpty, Always, TestLastFail (
+ [["get_drive_name"; "1"]]), [];
+ InitEmpty, Always, TestLastFail (
+ [["get_drive_name"; "99"]]), [];
+ ];
+ shortdesc = "get the drive name";
+ longdesc = "\
+Return the optional C<name> field passed to C<guestfs_add_drive_opts>
+when n'th drive C<index> was added.
+
+If there is no C<name> field associated with the drive
+then this function returns an error, with error code C<ESRCH>.
+
+If you only have a device name and not an index, you can call
+C<guestfs_device_index>. If what you want instead is the
+drive name as it appears to libguestfs, there is no function
+for that, instead use C<\"/dev/sda\">, C<\"/dev/sdb\">
etc." };
+
]
let daemon_functions = [
diff --git a/lib/drives.c b/lib/drives.c
index fd95308d2d..0da730c734 100644
--- a/lib/drives.c
+++ b/lib/drives.c
@@ -31,6 +31,7 @@
#include <netdb.h>
#include <arpa/inet.h>
#include <assert.h>
+#include <errno.h>
#include <libintl.h>
#include "c-ctype.h"
@@ -594,6 +595,25 @@ guestfs_int_free_drives (guestfs_h *g)
g->nr_drives = 0;
}
+char *
+guestfs_impl_get_drive_name (guestfs_h *g, int i)
+{
+ const char *name;
+
+ if (i < 0 || i > g->nr_drives) {
The upper bound check is off by one; should be ">=".
+ guestfs_int_error_errno (g, EINVAL, _("drive index out of
range"));
+ return NULL;
+ }
+
+ name = g->drives[i]->name;
Based on the comments in "lib/guestfs-internal.h" and on the
ITER_DRIVES() macro:
/* Array of drives added by add-drive* APIs.
*
* Before launch this list can be empty or contain some drives.
*
* During launch, a dummy slot may be added which represents the
* slot taken up by the appliance drive.
*
* During shutdown, this list is deleted, so that each launch gets a
* fresh set of drives (however callers: don't do this, create a new
* handle each time).
*
* Always use ITER_DRIVES macro to iterate over this list!
*/
struct drive **drives;
size_t nr_drives;
#define ITER_DRIVES(g,i,drv) \
for (i = 0; i < (g)->nr_drives; ++i) \
if (((drv) = (g)->drives[i]) != NULL)
I think we should check whether "g->drives[i]" is NULL, before we check
"g->drives[i]->name".
Otherwise, I'm OK with this patch (still need to look at the alternative
you psoted).
Thanks
Laszlo
+ if (!name) {
+ guestfs_int_error_errno (g, ESRCH, _("drive does not have a name"));
+ return NULL;
+ }
+
+ return safe_strdup (g, name);
+}
+
/**
* Check string parameter matches regular expression
* C<^[-_[:alnum:]]+$> (in C locale).
diff --git a/tests/c-api/tests-main.c b/tests/c-api/tests-main.c
index 9e01e4a3dc..a0aad1735d 100644
--- a/tests/c-api/tests-main.c
+++ b/tests/c-api/tests-main.c
@@ -431,7 +431,9 @@ create_handle (void)
exit (EXIT_FAILURE);
}
- if (guestfs_add_drive_scratch (g, INT64_C(2)*1024*1024*1024, -1) == -1) {
+ if (guestfs_add_drive_scratch (g, INT64_C(2)*1024*1024*1024,
+ GUESTFS_ADD_DRIVE_SCRATCH_NAME, "test",
+ -1) == -1) {
printf ("FAIL: guestfs_add_drive_scratch\n");
exit (EXIT_FAILURE);
}
1281
days inactive
1282
days old
5 comments
2 participants
participants (2)
-
Laszlo Ersek -
Richard W.M. Jones