Tomáš Golembiovský
2016-Aug-01 12:46 UTC
[Libguestfs] [PATCH] v2v: fixed file_owner function
What was happening in file_owner function did not match the description
in the comment. When a path is owned by multiple packages the returned
string was in fact a concatenation of the names of all packages that own
it. E.g. for `Linux.is_file_owned g inspect "/etc"` the returned value
was "filesystemyum" (i.e. "filesystem" + "yum").
Signed-off-by: Tom?? Golembiovsk? <tgolembi at redhat.com>
---
v2v/linux.ml | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/v2v/linux.ml b/v2v/linux.ml
index d20194b..aeff5c5 100644
--- a/v2v/linux.ml
+++ b/v2v/linux.ml
@@ -99,14 +99,18 @@ let rec file_owner g inspect path (* Although it is
possible in RPM for multiple packages to own
* a file, this deliberately only returns one package.
*)
- let cmd = [| "rpm"; "-qf"; "--qf";
"%{NAME}"; path |] in
+ let cmd = [| "rpm"; "-qf"; "--qf";
"%{NAME}\\n"; path |] in
debug "%s" (String.concat " " (Array.to_list cmd));
- (try g#command cmd
+ (try
+ let pkgs = g#command_lines cmd in
+ pkgs.(0)
with Guestfs.Error msg as exn ->
if String.find msg "is not owned" >= 0 then
raise Not_found
else
raise exn
+ | Invalid_argument msg ->
+ raise Not_found
)
| format ->
--
2.9.2
On Monday, 1 August 2016 14:46:14 CEST Tom?? Golembiovsk? wrote:> What was happening in file_owner function did not match the description > in the comment. When a path is owned by multiple packages the returned > string was in fact a concatenation of the names of all packages that own > it. E.g. for `Linux.is_file_owned g inspect "/etc"` the returned value > was "filesystemyum" (i.e. "filesystem" + "yum"). > > Signed-off-by: Tom?? Golembiovsk? <tgolembi at redhat.com> > --- > v2v/linux.ml | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/v2v/linux.ml b/v2v/linux.ml > index d20194b..aeff5c5 100644 > --- a/v2v/linux.ml > +++ b/v2v/linux.ml > @@ -99,14 +99,18 @@ let rec file_owner g inspect path > (* Although it is possible in RPM for multiple packages to own > * a file, this deliberately only returns one package. > *) > - let cmd = [| "rpm"; "-qf"; "--qf"; "%{NAME}"; path |] in > + let cmd = [| "rpm"; "-qf"; "--qf"; "%{NAME}\\n"; path |] in > debug "%s" (String.concat " " (Array.to_list cmd)); > - (try g#command cmd > + (try > + let pkgs = g#command_lines cmd in > + pkgs.(0) > with Guestfs.Error msg as exn -> > if String.find msg "is not owned" >= 0 then > raise Not_found > else > raise exn > + | Invalid_argument msg -> > + raise Not_foundJust fixed the indentation of this "raise", but otherwise this patch LGTM. Pushed, thank you. -- Pino Toscano -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: This is a digitally signed message part. URL: <http://listman.redhat.com/archives/libguestfs/attachments/20160801/7af0ec2c/attachment.sig>
Richard W.M. Jones
2016-Aug-03 10:14 UTC
Re: [Libguestfs] [PATCH] v2v: fixed file_owner function
On Mon, Aug 01, 2016 at 02:46:14PM +0200, Tomáš Golembiovský wrote:> What was happening in file_owner function did not match the description > in the comment. When a path is owned by multiple packages the returned > string was in fact a concatenation of the names of all packages that own > it. E.g. for `Linux.is_file_owned g inspect "/etc"` the returned value > was "filesystemyum" (i.e. "filesystem" + "yum"). > > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> > --- > v2v/linux.ml | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/v2v/linux.ml b/v2v/linux.ml > index d20194b..aeff5c5 100644 > --- a/v2v/linux.ml > +++ b/v2v/linux.ml > @@ -99,14 +99,18 @@ let rec file_owner g inspect path > (* Although it is possible in RPM for multiple packages to own > * a file, this deliberately only returns one package. > *) > - let cmd = [| "rpm"; "-qf"; "--qf"; "%{NAME}"; path |] in > + let cmd = [| "rpm"; "-qf"; "--qf"; "%{NAME}\\n"; path |] in > debug "%s" (String.concat " " (Array.to_list cmd)); > - (try g#command cmd > + (try > + let pkgs = g#command_lines cmd in > + pkgs.(0) > with Guestfs.Error msg as exn -> > if String.find msg "is not owned" >= 0 then > raise Not_found > else > raise exn > + | Invalid_argument msg -> > + raise Not_foundMy concern/confusion about this patch is what raises Invalid_argument? I'm assuming it's the array index, which could raise `Invalid_argument "index out of bounds"' if the g#command_lines returns a zero-length list, but then my question would be why is the rpm command not returning any lines? Even if this can happen, the exception shouldn't be converted to Not_found. It should be an internal error or the exception should be left unchanged. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
Tomáš Golembiovský
2016-Aug-03 10:51 UTC
Re: [Libguestfs] [PATCH] v2v: fixed file_owner function
On Wed, 3 Aug 2016 11:14:26 +0100 "Richard W.M. Jones" <rjones@redhat.com> wrote:> On Mon, Aug 01, 2016 at 02:46:14PM +0200, Tomáš Golembiovský wrote: > > What was happening in file_owner function did not match the description > > in the comment. When a path is owned by multiple packages the returned > > string was in fact a concatenation of the names of all packages that own > > it. E.g. for `Linux.is_file_owned g inspect "/etc"` the returned value > > was "filesystemyum" (i.e. "filesystem" + "yum"). > > > > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> > > --- > > v2v/linux.ml | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/v2v/linux.ml b/v2v/linux.ml > > index d20194b..aeff5c5 100644 > > --- a/v2v/linux.ml > > +++ b/v2v/linux.ml > > @@ -99,14 +99,18 @@ let rec file_owner g inspect path > > (* Although it is possible in RPM for multiple packages to own > > * a file, this deliberately only returns one package. > > *) > > - let cmd = [| "rpm"; "-qf"; "--qf"; "%{NAME}"; path |] in > > + let cmd = [| "rpm"; "-qf"; "--qf"; "%{NAME}\\n"; path |] in > > debug "%s" (String.concat " " (Array.to_list cmd)); > > - (try g#command cmd > > + (try > > + let pkgs = g#command_lines cmd in > > + pkgs.(0) > > with Guestfs.Error msg as exn -> > > if String.find msg "is not owned" >= 0 then > > raise Not_found > > else > > raise exn > > + | Invalid_argument msg -> > > + raise Not_found > > My concern/confusion about this patch is what raises Invalid_argument? > > I'm assuming it's the array index, which could raise `Invalid_argument > "index out of bounds"' if the g#command_lines returns a zero-length > list, but then my question would be why is the rpm command not > returning any lines? > > Even if this can happen, the exception shouldn't be converted to > Not_found. It should be an internal error or the exception should be > left unchanged.Yes, it is to catch the case when rpm reports nothing. Don't know if/when this can happen. Do you prefer an error in the exception handler (instead of raise Not_found), or should I check the array length (and report error) before accessing it?> > Rich. > > -- > Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones > Read my programming and virtualization blog: http://rwmj.wordpress.com > virt-p2v converts physical machines to virtual machines. Boot with a > live CD or over the network (PXE) and turn machines into KVM guests. > http://libguestfs.org/virt-v2v-- Tomáš Golembiovský <tgolembi@redhat.com>